Modify

Opened 6 weeks ago

Closed 3 weeks ago

Last modified 3 weeks ago

#18523 closed defect (fixed)

[PATCH] Upload dialog no longer displays number of changes in changeset after second upload

Reported by: anonymous Owned by: Don-vip
Priority: normal Milestone: 20.01
Component: Core Version: tested
Keywords: template_report regression upload changeset Cc: taylor.smock, sanchi

Description (last modified by Don-vip)

What steps will reproduce the problem?

  1. Make some changes, upload changeset
  2. Make some more changes, upload changeset

What is the expected result?

Under the data source it should say
'Uploading # objects to # changeset using # request

Objects are uploaded to a new changeset. The changeset is going to be closed after this upload'

What happens instead?

The count and new changeset comment are missing from all following uploads after the first one. To see these again it is required to quit/restart josm, reload your data, redo the changes, and upload from there.

Reverting to a pervious version fixes this issue.

Please provide any additional information below. Attach a screenshot if possible.


URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2020-01-02 22:34:59 +0100 (Thu, 02 Jan 2020)
Build-Date:2020-01-02 21:52:31
Revision:15628
Relative:URL: ^/trunk

Identification: JOSM/1.5 (15628 en) Mac OS X 10.14.6
OS Build number: Mac OS X 10.14.6 (18G1012)
Memory Usage: 726 MB / 1820 MB (223 MB allocated, but free)
Java version: 1.8.0_231-b11, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: Display 188945226 1920x1080, Display 69944052 2048x1152
Maximum Screen Size: 2048x1152
VM arguments: [-Djava.security.policy=file:<java.home>/lib/security/javaws.policy, -DtrustProxy=true, -Djnlpx.home=<java.home>/bin, -Djava.security.manager, -Djnlpx.origFilenameArg=${HOME}/Library/Application Support/Oracle/Java/Deployment/cache/6.0/56/1ee8cfb8-4e1b3343, -Djnlpx.remove=false, -Dsun.awt.warmup=true, -Djava.util.Arrays.useLegacyMergeSort=true, -Djnlpx.heapsize=NULL,2048m, -Dmacosx.jnlpx.dock.name=JOSM, -Dmacosx.jnlpx.dock.icon=${HOME}/Library/Application Support/Oracle/Java/Deployment/cache/6.0/25/4c122699-23c26c79.icns, -Djnlp.application.href=https://josm.openstreetmap.de/download/josm.jnlp , -Djnlpx.jvm="<java.home>/bin/java"]

Plugins:
+ auto_tools (73)
+ markseen (14)
+ osm-obj-info (56)
+ scripting (30796)
+ utilsplugin2 (35248)


Last errors/warnings:
- W: java.nio.file.NoSuchFileException: ${HOME}/Downloads/kaart.durazno.validator.mapcss
- E: Skipping to the next rule, because of an error:
- E: org.openstreetmap.josm.gui.mappaint.mapcss.parsergen.ParseException: Encountered " "(" "( "" at line 9, column 14.
- W: No configuration settings found.  Using hardcoded default values for all pools.
- W: Region [TMS_BLOCK_v2] Resetting cache
- E: Communication with OSM server failed - org.openstreetmap.josm.gui.widgets.HtmlPanel[,0,0,0x0,invalid,layout=java.awt.BorderLayout,alignmentX=0.0,alignmentY=0.0,border=,flags=9,maximumSize=,minimumSize=,preferredSize=]
- W: Proposed rect has such extreme aspect ratio that it would be zero-width at preferredZoom

Attachments (13)

changeset_issue.png (125.3 KB) - added by anonymous 6 weeks ago.
18523.patch (2.6 KB) - added by taylor.smock 6 weeks ago.
Initial patch that "fixes" the problem
18523.1.patch (2.6 KB) - added by taylor.smock 5 weeks ago.
Switch from BorderLayout to GridBagLayout
layoutbug.jpg (39.4 KB) - added by Don-vip 4 weeks ago.
18523.GBC_BOTH.patch (899 bytes) - added by taylor.smock 4 weeks ago.
GBC.HORIZONTAL -> GBC.BOTH
2020-01-24-071411_598x465_scrot.png (11.1 KB) - added by simon04 4 weeks ago.
Screen Shot 2020-01-24 at 7.25.45 AM.png (133.3 KB) - added by taylor.smock 4 weeks ago.
all.do_not_apply.patch (328.8 KB) - added by taylor.smock 4 weeks ago.
All of the modifications in my josm/core directory
2020-01-25-201141_629x544_scrot.png (11.9 KB) - added by simon04 4 weeks ago.
Looks good
18523.png (25.0 KB) - added by Klumbumbus 3 weeks ago.
18523.manually_set_minimum_size.patch (1.0 KB) - added by taylor.smock 3 weeks ago.
18523.ui_revert.patch (3.6 KB) - added by taylor.smock 3 weeks ago.
Revert UI while leaving logic (advanced pref upload.source.obtainautomatically can be set)
18523.revert_gridbaglayout_add_seemingly_unnecessary_call.patch (1.9 KB) - added by taylor.smock 3 weeks ago.
Go from GridBagLayout -> BorderLayout and add a totally unnecessary method call (super.getPreferredSize()), if it didn't seem to fix a UI issue.

Download all attachments as: .zip

Change History (54)

Changed 6 weeks ago by anonymous

Attachment: changeset_issue.png added

comment:1 Changed 6 weeks ago by skyper

Keywords: regression upload changeset added
Version: tested

comment:2 Changed 6 weeks ago by Don-vip

Cc: taylor.smock added
Description: modified (diff)
Summary: Dialogue box no longer displays number of changes in changeset after second uploadUpload dialog no longer displays number of changes in changeset after second upload

comment:3 Changed 6 weeks ago by Don-vip

Description: modified (diff)

comment:4 in reply to:  description Changed 6 weeks ago by taylor.smock

It looks like the minimum height for the pnlUploadParameterSummary is being set to 0. I've got a quick and dirty workaround, but I'm not certain what the best way workaround would actually be.

comment:5 Changed 6 weeks ago by taylor.smock

Summary: Upload dialog no longer displays number of changes in changeset after second upload[PATCH] Upload dialog no longer displays number of changes in changeset after second upload

comment:6 Changed 6 weeks ago by Don-vip

Milestone: 20.01

Changed 6 weeks ago by taylor.smock

Attachment: 18523.patch added

Initial patch that "fixes" the problem

Changed 5 weeks ago by taylor.smock

Attachment: 18523.1.patch added

Switch from BorderLayout to GridBagLayout

comment:7 Changed 4 weeks ago by taylor.smock

Ping?

I'd like to know if I should try to find a better solution, or if switching to GridBagLayout is acceptable.

comment:8 Changed 4 weeks ago by Don-vip

Owner: changed from team to Don-vip
Status: newassigned

I guess GridBagLayout is fine. Let's give it a try.

comment:9 Changed 4 weeks ago by Don-vip

Resolution: fixed
Status: assignedclosed

In 15752/josm:

fix #18523 - Upload dialog: Switch from BorderLayout to GridBagLayout (patch by taylor.smock)

comment:10 Changed 4 weeks ago by Don-vip

Resolution: fixed
Status: closedreopened

Nope. I didn't have any problem before applying this patch and now the layout does not work anymore for me:

Changed 4 weeks ago by Don-vip

Attachment: layoutbug.jpg added

comment:11 in reply to:  10 Changed 4 weeks ago by taylor.smock

Replying to Don-vip:

Nope. I didn't have any problem before applying this patch and now the layout does not work anymore for me:

The UI was working for me (as in, I didn't have it cut off).

If I comment out just about any of the adds in build, buildUploadCommentPanel, pnlUploadParameterSummary, or cbRequestReview, it works for me again without any of the patches here applied.

Can you check if changing the GBC.HORIZONTAL to GBC.BOTH works? (in build). And vice versa? (also, I've noticed that I've had to run ant clean dist to get some changes to show up, but that probably wasn't your problem).

Changed 4 weeks ago by taylor.smock

Attachment: 18523.GBC_BOTH.patch added

GBC.HORIZONTAL -> GBC.BOTH

Changed 4 weeks ago by simon04

Changed 4 weeks ago by taylor.smock

Changed 4 weeks ago by taylor.smock

Attachment: all.do_not_apply.patch added

All of the modifications in my josm/core directory

comment:12 Changed 4 weeks ago by taylor.smock

The screen shot is from the 18523.GBC_BOTH.patch.

I've uploaded a svn diff of everything in my josm/core directory as well.

I think the only things that would affect this are between lines 2805 and 2832, and the diff in src/org/openstreetmap/josm/gui/io/UploadDialog.java was just to ensure that I always got a new upload dialog for testing purposes, but I'm not encountering the issue at all, which means I cannot verify if I have fixed it or not.

I've also tested in a josm directory with literally nothing else applied (just attachment:18523.GBC_BOTH.patch), and that didn't show the problem.

Last edited 4 weeks ago by taylor.smock (previous) (diff)

comment:13 Changed 4 weeks ago by taylor.smock

@simon04:
I reproduced your screenshot on Fedora 31.

The 18523.GBC_BOTH.patch fixed it for me. Can you check if it fixes it for you as well?

Funny things are still happening though.

@Don-vip, should I make a patch that reverts the UI, but leaves an advanced preference for getting sources automatically, until I (or someone else) figures out the UI issue?

Changed 4 weeks ago by simon04

Looks good

comment:14 Changed 3 weeks ago by Klumbumbus

Priority: normalcritical

(This is a pretty bad bug. I wonder why we don't receive more bug reports.)

comment:15 Changed 3 weeks ago by Don-vip

Priority: criticalnormal

nothing critical, it's just a display bug.

comment:16 Changed 3 weeks ago by Klumbumbus

I need to resize the upload window at every upload (except the first), because it is unusable:

Changed 3 weeks ago by Klumbumbus

Attachment: 18523.png added

comment:17 Changed 3 weeks ago by Don-vip

Annoying, but no critical, as you can still upload.

comment:18 Changed 3 weeks ago by Don-vip

Resolution: fixed
Status: reopenedclosed

In 15778/josm:

fix #18523 - fix upload dialog layout (patch by taylor.smock)

comment:19 Changed 3 weeks ago by skyper

Resolution: fixed
Status: closedreopened

Still have the same problem as Klumbumbus in comment 10. Switching to another tab and back solves the issue but on all uploads except the first one the source text box is too small in the beginning.

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2020-01-28 00:39:43 +0100 (Tue, 28 Jan 2020)
Revision:15791
Build-Date:2020-01-28 02:30:55
URL:https://josm.openstreetmap.de/svn/trunk

comment:20 Changed 3 weeks ago by GerdP

Ticket #18636 has been marked as a duplicate of this ticket.

Changed 3 weeks ago by taylor.smock

comment:21 Changed 3 weeks ago by taylor.smock

Can everyone having the problem shown in comment:10 try the attachment:18523.manually_set_minimum_size.patch ? From what I understand of Swing, it shouldn't be necessary, but its working for me on multiple trial uploads.

I didn't notice it with the previous patch because I had changed UploadDialog to always return a new upload dialog so I didn't have to restart JOSM every time I made a change.

comment:22 Changed 3 weeks ago by GerdP

Yes, fixes the problem for me. My way to reproduce it:
1) Create node with amenity=parking
2) "Upload data" (dialog is OK)
3) press Esc
4) "upload data" again (dialog is not OK)

comment:23 in reply to:  22 Changed 3 weeks ago by taylor.smock

That is basically how I reproduced it once I was no longer getting a "new" upload dialog every time.

comment:24 in reply to:  22 Changed 3 weeks ago by Klumbumbus

18523.manually_set_minimum_size.patch fixes the problem for me too.

comment:25 Changed 3 weeks ago by simon04

Cc: sanchi added

comment:26 Changed 3 weeks ago by simon04

With the follwoing parts of r15752 reverted, I cannot reproduce according to comment:22

  • src/org/openstreetmap/josm/gui/io/BasicUploadSettingsPanel.java

    diff --git a/src/org/openstreetmap/josm/gui/io/BasicUploadSettingsPanel.java b/src/org/openstreetmap/josm/gui/io/BasicUploadSettingsPanel.java
    index 2b6a61ec7..1135540d1 100644
    a b  
    33
    44import static org.openstreetmap.josm.tools.I18n.tr;
    55
     6import java.awt.BorderLayout;
    67import java.awt.GridBagLayout;
    78import java.awt.event.ActionEvent;
    89import java.awt.event.ActionListener;
    protected void discardAllUndoableEdits() { 
    176177    }
    177178
    178179    protected void build() {
    179         setLayout(new GridBagLayout());
     180        setLayout(new BorderLayout());
    180181        setBorder(BorderFactory.createEmptyBorder(3, 3, 3, 3));
    181         add(buildUploadCommentPanel(), GBC.eol().fill(GBC.BOTH));
    182         add(pnlUploadParameterSummary, GBC.eol().fill(GBC.BOTH));
    183         add(cbRequestReview, GBC.eol().fill(GBC.BOTH));
     182        add(buildUploadCommentPanel(), BorderLayout.NORTH);
     183        add(pnlUploadParameterSummary, BorderLayout.CENTER);
     184        add(cbRequestReview, BorderLayout.SOUTH);
    184185        cbRequestReview.addItemListener(e -> changesetReviewModel.setReviewRequested(e.getStateChange() == ItemEvent.SELECTED));
    185186    }
    186187

comment:27 Changed 3 weeks ago by Don-vip

I would also try to revert things instead of adding new code. The upload dialog was working fine for years before r15600 + r15752.

comment:28 in reply to:  26 Changed 3 weeks ago by taylor.smock

Switching back to BorderLayout also works on Mac (at least on this one), without the 18523.manually_set_minimum_size.patch.

The hard part about this bug (from my perspective) is that different machines are acting differently with respect to the UI.

For example, I wasn't seeing the issue that Don-vip was seeing in comment:10 on Mac, while I saw it on Fedora.

I'll post a patch that reverts the UI portion, while keeping an advanced option around (for whenever I or someone else figures out the UI issues).

Changed 3 weeks ago by taylor.smock

Attachment: 18523.ui_revert.patch added

Revert UI while leaving logic (advanced pref upload.source.obtainautomatically can be set)

comment:29 Changed 3 weeks ago by taylor.smock

That should fix the UI issue while keeping the functionality for an advanced preference.

Possible issues:

  • I left the code for generating the UI in JOSM, so there are some unused variables (I want to see if I can get something that satisfies everyone)
  • People may be confused about missing the checkbox while still getting source automatically

My initial patch was literally setting the minimum size for the affected panel to 80, which the manually_set_minimum_size patch did, but the latter was a bit better in terms of (hopefully) dealing with HiDPI.

comment:30 in reply to:  22 ; Changed 3 weeks ago by mdk

Replying to GerdP:

Yes, fixes the problem for me. My way to reproduce it:
1) Create node with amenity=parking
2) "Upload data" (dialog is OK)
3) press Esc
4) "upload data" again (dialog is not OK)

I could reproduce this with r15793. But I found an interesting detail (on Windows):
When I go to the border of the dialog where the resize mouse pointer appears and I just klick, the layout will be corrected!

comment:31 in reply to:  30 Changed 3 weeks ago by sanchi

I could reproduce this with r15793. But I found an interesting detail (on Windows):
When I go to the border of the dialog where the resize mouse pointer appears and I just klick, the layout will be corrected!

In mac it happens the same

comment:32 Changed 3 weeks ago by taylor.smock

The following also fixed the problem for me. Yes, all I am doing is calling super.getPreferredSize() -- I was going to check and see if that was giving odd results as well. It turns out that is relatively stable. (On initial show getPreferredSize gets called four times, with 190x434, 222x434, and 299x434 being the different minimum sizes, after initial show it gets called once, which returns 222x434 -- heightxwidth).

  • src/org/openstreetmap/josm/gui/io/UploadDialog.java

     
    432432    static final class CompactTabbedPane extends JTabbedPane {
    433433        @Override
    434434        public Dimension getPreferredSize() {
     435            super.getPreferredSize();
    435436            // make sure the tabbed pane never grabs more space than necessary
    436437            return super.getMinimumSize();
    437438        }

I hate working on UI stuff. I don't get why there would be a different behavior if you call super.getPreferredSize() before super.getMinimumSize() versus just super.getMinimumSize() -- it makes no logical sense to me. There is probably something somewhere in the code that will clear it up though.

Last edited 3 weeks ago by taylor.smock (previous) (diff)

comment:33 Changed 3 weeks ago by skyper

Maybe the tabs with possibly different height is the problem. Switching to "Changesets" or "Advanced" adjusts the height of the lower part of the dialog, but only after changing a setting there.

comment:34 in reply to:  33 Changed 3 weeks ago by taylor.smock

Replying to skyper:

Maybe the tabs with possibly different height is the problem. Switching to "Changesets" or "Advanced" adjusts the height of the lower part of the dialog, but only after changing a setting there.

Possibly -- the tabs changing height appears to be a byproduct of GridBagLayout -- BorderLayout doesn't have that problem (see r15752). I've switched that back (essentially did the patch from comment:28), and it doesn't occur anymore.

I'm also not seeing the UI issue from comment:10 when I run JOSM from Eclipse, but I do see it when I run from a jar file.

That being said, my observance from comment:32 also fixes the UI issue (when run from a jar file).

I don't know why.

super.getPreferredSize(); // This probably fixes #18523. Don't know why. Don't know how. It just does.

Changed 3 weeks ago by taylor.smock

Go from GridBagLayout -> BorderLayout and add a totally unnecessary method call (super.getPreferredSize()), if it didn't seem to fix a UI issue.

comment:35 Changed 3 weeks ago by Don-vip

Resolution: fixed
Status: reopenedclosed

In 15802/josm:

fix #18523 - fix upload dialog layout (patch by taylor.smock)

comment:36 in reply to:  35 Changed 3 weeks ago by taylor.smock

Replying to Don-vip:

In 15802/josm:

fix #18523 - fix upload dialog layout (patch by taylor.smock)

Did it really fix it for everyone? I probably should have indicated that people having the issue should test it.

comment:37 Changed 3 weeks ago by Don-vip

Let's see tomorrow...

comment:38 in reply to:  35 ; Changed 3 weeks ago by skyper

Replying to Don-vip:

In 15802/josm:

fix #18523 - fix upload dialog layout (patch by taylor.smock)

Work like a charm now thought the height always stays the same.

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2020-01-31 00:23:28 +0100 (Fri, 31 Jan 2020)
Revision:15803
Build-Date:2020-01-31 02:30:58
URL:https://josm.openstreetmap.de/svn/trunk

Identification: JOSM/1.5 (15803 en) Linux Debian GNU/Linux 10 (buster)
Last edited 3 weeks ago by skyper (previous) (diff)

comment:39 in reply to:  38 Changed 3 weeks ago by taylor.smock

Replying to skyper:

Work like a charm now thought the height always stays the same.

r15598 had the same behavior (at least on my machine). So we are back to the previous behavior.

I still don't know why the call to getPreferredSize when I did nothing with the output fixed the issue. Java bug maybe? I would have expected the behavior of getMinimumSize to be the same, regardless of whether or not getPreferredSize was called first.

comment:40 Changed 3 weeks ago by GerdP

I also cannot reproduce the problem with r15803 (tried Oracle Java and OpenJDK 1.8)

comment:41 Changed 3 weeks ago by Klumbumbus

Works fine for me

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2020-01-31 00:23:28 +0100 (Fri, 31 Jan 2020)
Build-Date:2020-01-31 02:30:58
Revision:15803
Relative:URL: ^/trunk

Identification: JOSM/1.5 (15803 de) Windows 10 64-Bit
OS Build number: Windows 10 Pro 1903 (18362)
Memory Usage: 952 MB / 1820 MB (531 MB allocated, but free)
Java version: 1.8.0_241-b07, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: \Display0 1680x1050
Maximum Screen Size: 1680x1050
VM arguments: [-Djava.security.manager, -Djava.security.policy=file:<java.home>\lib\security\javaws.policy, -DtrustProxy=true, -Djnlpx.home=<java.home>\bin, -Djnlpx.origFilenameArg=C:\Program Files (x86)\josm-latest.jnlp, -Djnlpx.remove=false, -Djava.util.Arrays.useLegacyMergeSort=true, -Djnlpx.heapsize=NULL,2048m, -Djnlpx.splashport=53153, -Djnlpx.jvm=<java.home>\bin\javaw.exe]
Dataset consistency test: No problems found

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Don-vip.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.