[rfc][icedtea-web] make download indicator more compact
Jiri Vanek
jvanek at redhat.com
Mon Jan 7 06:41:35 PST 2013
On 01/04/2013 05:35 PM, Adam Domurad wrote:
> On 01/03/2013 11:07 AM, Jiri Vanek wrote:
>> Hi!
>>
>> This patch is adding "show details" to download indicator. For One jar jnlp files it behaviour is
>> unaffected. For Multiple jars there is just one progress bar, but can be shown "old stile"
>> detailed one via clicking to "Show details". Then can be minimalised by "hide details". Button is
>> localised.
>>
>> The button is nasty not nice.. Any better idea? but still this is better then previous approach.
>>
>> Looking forward to have this inside!
>>
>> Best rigards
>> J.
>
> Thanks for looking into this! I was actually comparing our download indicator to proprietary just
> yesterday, while staring at multiple jars downloading. The average user does not want to know about
> the jars being downloaded, in fact I think such information can look scary :-).
>
>> diff -r 9549226afa8f netx/net/sourceforge/jnlp/cache/DefaultDownloadIndicator.java
>> --- a/netx/net/sourceforge/jnlp/cache/DefaultDownloadIndicator.java Thu Jan 03 09:54:16 2013 +0100
>> +++ b/netx/net/sourceforge/jnlp/cache/DefaultDownloadIndicator.java Thu Jan 03 17:07:28 2013 +0100
>> @@ -62,6 +62,7 @@
>>
>> /** shared constraint */
>> static GridBagConstraints vertical;
>> + static GridBagConstraints verticalNoClean;
>> static GridBagConstraints verticalIndent;
>> static {
>> vertical = new GridBagConstraints();
>> @@ -70,8 +71,12 @@
>> vertical.fill = GridBagConstraints.HORIZONTAL;
>> vertical.anchor = GridBagConstraints.WEST;
>>
>> + verticalNoClean = new GridBagConstraints();
>> + verticalNoClean.weightx = 1.0;
>> +
>> verticalIndent = (GridBagConstraints) vertical.clone();
>> verticalIndent.insets = new Insets(0, 10, 3, 0);
>> +
>
> Nit: These two blank lines (and all the ones added below) have tabs :-)
>
>> }
>>
>> /**
>> @@ -114,15 +119,15 @@
>>
>> frame.getContentPane().add(result, vertical);
>> frame.pack();
>> -
>> if (!frame.isVisible()) {
>> - Dimension screenSize = Toolkit.getDefaultToolkit().getScreenSize();
>> - Insets insets =
>> Toolkit.getDefaultToolkit().getScreenInsets(frame.getGraphicsConfiguration());
>> - Dimension screen = new Dimension(screenSize.width - insets.left,
>> - screenSize.height - insets.top);
>> - frame.setLocation(screen.width - frame.getWidth(),
>> - screen.height - frame.getHeight());
>> + placeFrameToLowerRight();
>> }
>> + result.addComponentListener(new ComponentAdapter() {
>> + @Override
>> + public void componentResized(ComponentEvent e) {
>> + placeFrameToLowerRight();
>> + }
>> + });
>>
>> frame.setVisible(true);
>>
>> @@ -131,6 +136,32 @@
>> }
>>
>> /**
>> + * The insets are calculated differently during first appearance
>> + * and during another appearance in case of some configurations.
>> + *
>> + * So the first value is stored to avoid jumping of window during later packing
>> + * of frame.
>> + *
>> + * However the second calculation is more correct:(
>> + *
>> + * This is affecting only multiple monitors which have different assets.
>> + * The underlying issue is that Toolkit.getDefaultToolkit().getScreenInsets
>> + * is returning assets for "random" monitor.
>> + */
>> + Dimension screen = null;
>> +
>> + private void placeFrameToLowerRight() throws HeadlessException {
>> + if (screen == null) {
>> + Dimension screenSize = Toolkit.getDefaultToolkit().getScreenSize();
>> + Insets insets =
>> Toolkit.getDefaultToolkit().getScreenInsets(frame.getGraphicsConfiguration());
>> + screen = new Dimension(screenSize.width - insets.left,
>> + screenSize.height - insets.top);
>> + }
>> + frame.setLocation(screen.width - frame.getWidth(),
>> + screen.height - frame.getHeight());
>> + }
>> +
>> + /**
>> * Remove a download service listener that was obtained by
>> * calling the getDownloadListener method from the shared
>> * download info window.
>> @@ -163,12 +194,26 @@
>> * Groups the url progress in a panel.
>> */
>> static class DownloadPanel extends JPanel implements DownloadServiceListener {
>> -
>> + private static enum States{
>> + oneJAr, collapsed, detailed;
>
> Enums should be all-caps
>
>> +
>> + }
>> +
>> + private static final String DETAILS=R("ButShowDetails");
>> + private static final String HIDE_DETAILS=R("ButHideDetails");
>> /** the download name */
>> private String downloadName;
>>
>> /** Downloading part: */
>> private JLabel header = new JLabel();
>> + /** Show/hide details button: */
>> + private JButton details = new JButton(DETAILS);
>> + /** used instead of details button in case of one jar*/
>> + private JLabel delimiter = new JLabel("");
>> + /** all already created progress bars*/
>> + private List<ProgressPanel> progressPanels = new ArrayList<ProgressPanel>();
>> + private States state=States.oneJAr;
>> + private ProgressPanel mainProgressPanel;
>>
>> /** list of URLs being downloaded */
>> private List<URL> urls = new ArrayList<URL>();
>> @@ -176,6 +221,7 @@
>> /** list of ProgressPanels */
>> private List<ProgressPanel> panels = new ArrayList<ProgressPanel>();
>>
>> +
>
> Nit: two blank lines are unnecessary.
>
>> /**
>> * Create a new download panel for with the specified download
>> * name.
>> @@ -184,9 +230,35 @@
>> setLayout(new GridBagLayout());
>>
>> this.downloadName = downloadName;
>> - this.add(header, vertical);
>> + this.add(header, verticalNoClean);
>> header.setFont(header.getFont().deriveFont(Font.BOLD));
>> -
>> + this.add(delimiter, vertical);
>> + details.addActionListener(new ActionListener() {
>> + @Override
>> + public void actionPerformed(ActionEvent e) {
>> + if (state == States.detailed) {
>> + state = States.collapsed;
>> + details.setText(DETAILS);
>> + for (ProgressPanel progressPanel : progressPanels) {
>> + remove(progressPanel);
>> + }
>> + add(mainProgressPanel, verticalIndent);
>> + synchronized (frameMutex) {
>> + frame.pack();
>> + }
>> + } else {
>> + state = States.detailed;
>> + details.setText(HIDE_DETAILS);
>> + remove(mainProgressPanel);
>> + for (ProgressPanel progressPanel : progressPanels) {
>> + add(progressPanel, verticalIndent);
>> + }
>> + synchronized (frameMutex) {
>> + frame.pack();
>> + }
>> + }
>> + }
>> + });
>> setOverallPercent(0);
>> }
>>
>> @@ -196,14 +268,25 @@
>> protected void addProgressPanel(URL url, String version) {
>> if (!urls.contains(url)) {
>> ProgressPanel panel = new ProgressPanel(url, version);
>> -
>> - add(panel, verticalIndent);
>> + if (state != States.collapsed) {
>> + add(panel, verticalIndent);
>> + }
>> + progressPanels.add(panel);
>> + urls.add(url);
>> + panels.add(panel);
>> + if (panels.size() == 2){
>
> Small comment here would be good explaining why this 'magic number'
>
>> + remove(panels.get(0));
>> + remove(panels.get(1));
>> + remove(delimiter);
>> + add(details,vertical);
>> + mainProgressPanel=new ProgressPanel();
>> + add(mainProgressPanel, verticalIndent);
>> + state=States.collapsed;
>> + }
>> synchronized (frameMutex) {
>> frame.pack();
>> }
>>
>> - urls.add(url);
>> - panels.add(panel);
>> }
>> }
>>
>> @@ -219,10 +302,10 @@
>> addProgressPanel(url, version);
>>
>> setOverallPercent(overallPercent);
>> -
>> ProgressPanel panel = panels.get(urls.indexOf(url));
>> panel.setProgress(readSoFar, total);
>> panel.repaint();
>> +
>> }
>> };
>> SwingUtilities.invokeLater(r);
>> @@ -230,12 +313,28 @@
>>
>> /**
>> * Sets the overall percent completed.
>> + * should be called via invokeLater
>> */
>> public void setOverallPercent(int percent) {
>> // don't get whole string from resource and sub in
>> // values because it'll be doing a MessageFormat for
>> // each update.
>> header.setText(downloading + " " + downloadName + ": " + percent + "% " + complete +
>> ".");
>> + Container c = header.getParent();
>> + //we need to adapt nbboth panels and also frame to new length of header text
>
> nbboth > both
>
>> + while (c != null) {
>> + c.invalidate();
>> + c.validate();
>> + if (c instanceof Window){
>> + ((Window) c).pack();
>> + }
>> + c=c.getParent();
>> + }
>> +
>> + if (mainProgressPanel != null) {
>> + mainProgressPanel.setProgress(percent, 100);
>> + mainProgressPanel.repaint();
>> + }
>> }
>>
>> /**
>> @@ -276,12 +375,28 @@
>>
>> private long total;
>> private long readSoFar;
>> + private Dimension size = new Dimension(80, 15);
>>
>> + ProgressPanel() {
>> + bar.setMinimumSize(size);
>> + bar.setPreferredSize(size);
>> + bar.setOpaque(false);
>> +
>> + setLayout(new GridBagLayout());
>> +
>> + GridBagConstraints gbc = new GridBagConstraints();
>> + styleGridBagConstraints(gbc);
>> + add(bar, gbc);
>> + }
>> +
>> ProgressPanel(URL url, String version) {
>> - JLabel location = new JLabel(" " + url.getHost() + "/" + url.getFile());
>> + this(" " + url.getHost() + "/" + url.getFile(),version);
>> + }
>> + ProgressPanel(String caption, String version) {
>> + JLabel location = new JLabel(caption);
>>
>> - bar.setMinimumSize(new Dimension(80, 15));
>> - bar.setPreferredSize(new Dimension(80, 15));
>> + bar.setMinimumSize(size);
>> + bar.setPreferredSize(size);
>> bar.setOpaque(false);
>>
>> setLayout(new GridBagLayout());
>> @@ -291,12 +406,8 @@
>> gbc.fill = GridBagConstraints.NONE;
>> gbc.gridwidth = GridBagConstraints.RELATIVE;
>> add(bar, gbc);
>> -
>> - gbc.insets = new Insets(0, 3, 0, 0);
>> - gbc.weightx = 1.0;
>> - gbc.fill = GridBagConstraints.HORIZONTAL;
>> - gbc.gridwidth = GridBagConstraints.REMAINDER;
>> - gbc.anchor = GridBagConstraints.WEST;
>> +
>> + styleGridBagConstraints(gbc);
>> add(location, gbc);
>> }
>>
>> @@ -325,6 +436,14 @@
>> g.fillRect(x + 1, y + 1, divide - 1, h - 1);
>> }
>> }
>> +
>> + private void styleGridBagConstraints(GridBagConstraints gbc) {
>> + gbc.insets = new Insets(0, 3, 0, 0);
>> + gbc.weightx = 1.0;
>> + gbc.fill = GridBagConstraints.HORIZONTAL;
>> + gbc.gridwidth = GridBagConstraints.REMAINDER;
>> + gbc.anchor = GridBagConstraints.WEST;
>> + }
>> };
>>
>> }
>
> Code looks OK nits aside.
Thanx for nits. Should be fixed now
>
> As for button ... yes it is nasty. IMO maybe we can take different approach here. The people
> interested in how the individual jars are downloading would mainly be 1. us trying to fix jar
> loading issues, 2. developers looking into jar loading issues.
>
> So what if the ICEDTEAPLUGIN_DEBUG environment variable turned this on/off ? It can be argued it is
> a bit hard for developers to access, but it isn't any easier to access this information from oracle
> plugin.
I think I don't like this idea :(
I have added an attempt to use an icon, And I must say it is much better. Btw, I have created this
icon.. so no wonders here :) But looks quite cool :)
>
> As well, I would be very happy to see the loading bar integrated with the splash screen (if splash
> screen is on). I believe this would nicely complement the user friendliness of the splash screen.
You mean to sync progress on progress bar with the one in splash? Or also showing the downlaoded
resources somehow and show details button in splash?
Sounds reasonable, but definitely as different changeset.
Thanx for review an ideas,
J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fixedDownloadIndicatorDialog2.patch
Type: text/x-patch
Size: 11443 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130107/f4d4c30c/fixedDownloadIndicatorDialog2.patch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: magnifyGlass-small.png
Type: image/png
Size: 1008 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130107/f4d4c30c/magnifyGlass-small.png
More information about the distro-pkg-dev
mailing list