[patch] Notes progress bar shows no progress

[Klumbumbus]

What steps will reproduce the problem?

1. close some notes or add some comments
2. upload

What is the expected result?

progress bar displays progress (percent of number of uploaded notes)

What happens instead?

no progress displayed and if console is not open one does not know if josm got stuck or not

additional a text "uploading 4/30" or similar would be nice too.

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

beware! gif is long (more than 3 minutes) (added an other one) and boring ;)

17288.gif​

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2019-02-02 21:01:18 +0100 (Sat, 02 Feb 2019)
Build-Date:2019-02-04 02:30:51
Revision:14760
Relative:URL: ^/trunk

Identification: JOSM/1.5 (14760 de) Windows 10 64-Bit
OS Build number: Windows 10 Pro 1803 (17134)
Memory Usage: 720 MB / 1820 MB (183 MB allocated, but free)
Java version: 1.8.0_201-b09, 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=57305, -Djnlpx.jvm=<java.home>\bin\javaw.exe]
Dataset consistency test: No problems found

Plugins:
+ DirectUpload (34867)
+ HouseNumberTaggingTool (34867)
+ OpeningHoursEditor (34867)
+ PicLayer (34867)
+ apache-commons (34506)
+ apache-http (34632)
+ buildings_tools (34867)
+ editgpx (34867)
+ ejml (34389)
+ geojson (116)
+ geotools (34513)
+ imagery-xml-bounds (34803)
+ imagery_offset_db (34867)
+ jna (34867)
+ jogl (1.2.2)
+ jts (34524)
+ log4j (34527)
+ measurement (34867)
+ photo_geotagging (34867)
+ photoadjust (34867)
+ reltoolbox (34867)
+ reverter (34867)
+ rex (49)
+ tag2link (34867)
+ tageditor (34867)
+ tagging-preset-tester (34678)
+ terracer (34867)
+ turnlanes-tagging (280)
+ turnrestrictions (34867)
+ undelete (34877)
+ utilsplugin2 (34867)
+ wikipedia (v1.1.1)

Tagging presets:
+ https://josm.openstreetmap.de/josmfile?page=Presets/OneClick&zip=1
+ https://josm.openstreetmap.de/josmfile?page=Presets/StolpersteineLight&zip=1
+ %UserProfile%\Documents\OSM\josm\data\defaultpresets.xml
+ %UserProfile%\Documents\OSM\TestNew\newpresets.xml
+ https://josm.openstreetmap.de/josmfile?page=Presets/PhilippinesAddresses&zip=1
+ https://josm.openstreetmap.de/josmfile?page=Presets/NewTags&zip=1
+ https://josm.openstreetmap.de/josmfile?page=Presets/Heritage&zip=1

Map paint styles:
+ %UserProfile%\Documents\OSM\josm\styles\standard\elemstyles.mapcss
- https://josm.openstreetmap.de/josmfile?page=Styles/HiDPISupport&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/NewHighwayColors&zip=1
+ https://josm.openstreetmap.de/josmfile?page=Styles/Coloured_Streets&zip=1
- %UserProfile%\Documents\OSM\TestNew\newicons.mapcss
- https://josm.openstreetmap.de/josmfile?page=Styles/Modified&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Maxspeed&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Lane_and_Road_Attributes&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/sac_scale&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/ShowID&zip=1
- %UserProfile%\Documents\OSM\eigene styles\PriorityRoad\PriorityRoad_1.0.mapcss
- https://josm.openstreetmap.de/josmfile?page=Styles/LayerChecker&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Surface&style&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/ParkingLanes&style&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Sidewalks&style&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Cycleways&style&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Osmc&style&zip=1
- https://raw.githubusercontent.com/species/josm-preset-wheelchair/master/sidewalks_kerbs.mapcss
- https://josm.openstreetmap.de/josmfile?page=Styles/LitObjects&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Lit&style&zip=1
- %UserProfile%\Documents\OSM\eigene styles\Tourenplanung.mapcss
- %UserProfile%\Documents\OSM\eigene styles\SpecificBuildingValues\SpecificBuildingValues.mapcss
- https://josm.openstreetmap.de/josmfile?page=Styles/Coloured_buildings&zip=1
- https://github.com/bastik/mapcss-tools/raw/osm/mapnik2mapcss/osm-results/mapnik.zip
- %UserProfile%\Documents\OSM\eigene styles\area-symbol.zip
- http://www.freietonne.de/ft_icons/josm/FreieTonne_rules_presets_zip.php
- http://www.openrailwaymap.org/styles/standard.zip
- https://josm.openstreetmap.de/josmfile?page=Styles/MaxspeedIcons&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/DestinationSignRelation&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/ParkingLanes&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Incline&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/PTStops&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/AdvertisingStyle&zip=1
- https://www.dropbox.com/s/qo3ai47fpv241jf/Styles_Fixme_and_Notes.zip?raw=1
- https://github.com/gmgeo/osmic-josm-style/archive/master.zip
- https://josm.openstreetmap.de/josmfile?page=Styles/hazmat&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Coloured_Suburb&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Coloured_Postcode&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/RecyclingMaterials&zip=1
- %UserProfile%\Documents\OSM\eigene styles\maxspeed\maxspeed_2.9_01 basierend auf 2.7_02 Zahlen.mapcss
- https://josm.openstreetmap.de/josmfile?page=Styles/Bench&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/PublicTransportV2&zip=1
- %UserProfile%\Documents\OSM\eigene styles\colourtag\colourtag_1.0.mapcss
- https://josm.openstreetmap.de/josmfile?page=Styles/ColourTag&zip=1
- %UserProfile%\Downloads\coloured_kerbs_style.zip
- https://josm.openstreetmap.de/josmfile?page=Styles/Admin_Boundaries&zip=1
- https://raw.githubusercontent.com/species/josm-preset-traffic_sign_direction/master/direction.mapcss

Validator rules:
+ https://raw.githubusercontent.com/<user.name>n-a-bauer/josm-validators/master/mtb.validator.mapcss
- %UserProfile%\Documents\OSM\TestNew\germ.validator.mapcss
- https://josm.openstreetmap.de/josmfile?page=Rules/CzechRepublicAddressSystem&zip=1
+ %UserProfile%\Documents\OSM\TestNew\new.validator.mapcss
+ https://josm.openstreetmap.de/josmfile?page=Rules/GermanySpecific&zip=1
+ https://josm.openstreetmap.de/josmfile?page=Rules/OsmoseValidations&zip=1

Last errors/warnings:
- W: No configuration settings found.  Using hardcoded default values for all pools.

[mnalis]

can confirm this is still present in current testing (15492)

[gaben]

I'm not sure, but maybe it just this much:

src/org/openstreetmap/josm/actions/upload/UploadNotesTask.java

@@ -65 +65 @@
         protected void realRun() throws SAXException, IOException, OsmTransferException {
             ProgressMonitor monitor = progressMonitor.createSubTaskMonitor(ProgressMonitor.ALL_TICKS, false);
             OsmApi api = OsmApi.getOsmApi();
+            monitor.setTicksCount(noteData.getNotes().size());
             for (Note note : noteData.getNotes()) {
+                monitor.setCustomText(tr("Uploaded notes {0}/{1}", monitor.getTicks(), monitor.getTicksCount()));
                 if (isCanceled) {
                     Logging.info("Note upload interrupted by user");
                     break;
@@ -76 +78 @@
                         processNoteComment(monitor, api, note, comment);
                     }
                 }
+                monitor.worked(1);
             }
         }

[gaben]

This seems to work, please test before commit :)

src/org/openstreetmap/josm/actions/upload/UploadNotesTask.java

@@ -63 +63 @@
 
         @Override
         protected void realRun() throws SAXException, IOException, OsmTransferException {
-            ProgressMonitor monitor = progressMonitor.createSubTaskMonitor(ProgressMonitor.ALL_TICKS, false);
             OsmApi api = OsmApi.getOsmApi();
+            getProgressMonitor().setTicksCount(noteData.getNotes().size());
             for (Note note : noteData.getNotes()) {
+                getProgressMonitor().setCustomText(tr("Uploading notes {0}/{1}", getProgressMonitor().getTicks(), getProgressMonitor().getTicksCount()));
                 if (isCanceled) {
                     Logging.info("Note upload interrupted by user");
                     break;
@@ -73 +74 @@
                 for (NoteComment comment : note.getComments()) {
                     if (comment.isNew()) {
                         Logging.debug("found note change to upload");
-                        processNoteComment(monitor, api, note, comment);
+                        processNoteComment(getProgressMonitor(), api, note, comment);
                     }
                 }
+                getProgressMonitor().worked(1);
             }
         }

[gaben]

Any comment/suggestion on the provided patch?

[taylor.smock]

This is probably better than the current implementation (little UI feedback).

Comments:

• L69 is longer than 145 characters (ant pmd checkstyle)
• Non-modified notes are counted (noteData.getNodes.size()). This can probably be worked around by getting the modified notes first (e.g., noteData.getNotes().stream().filter(note -> note.getLastComment().isNew()).collect(Collectors.toList())), and using that as the collection.

Modified patch from comment:7 (I am going to need to test what happens with a new comment sometime), and I think that someone should write some tests for it (either gaben or myself, probably).

src/org/openstreetmap/josm/actions/upload/UploadNotesTask.java

@@ -4 +4 @@
 import static org.openstreetmap.josm.tools.I18n.tr;
 
 import java.io.IOException;
+import java.util.Collection;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Optional;
 import java.util.stream.Collectors;
 
 import javax.swing.JOptionPane;
@@ -63 +65 @@
 
         @Override
         protected void realRun() throws SAXException, IOException, OsmTransferException {
-            ProgressMonitor monitor = progressMonitor.createSubTaskMonitor(ProgressMonitor.ALL_TICKS, false);
             OsmApi api = OsmApi.getOsmApi();
-            for (Note note : noteData.getNotes()) {
+            final Collection<Note> modifiedNotes = noteData.getNotes().stream()
+                    .filter(note -> Optional.ofNullable(note.getLastComment()).map(NoteComment::isNew).orElse(false))
+                    .collect(Collectors.toList());
+            getProgressMonitor().setTicksCount(modifiedNotes.size());
+            for (Note note : modifiedNotes) {
+                getProgressMonitor().setCustomText(tr("Uploading notes {0}/{1}", getProgressMonitor().getTicks(),
+                        getProgressMonitor().getTicksCount()));
                 if (isCanceled) {
                     Logging.info("Note upload interrupted by user");
                     break;
@@ -73 +80 @@
                 for (NoteComment comment : note.getComments()) {
                     if (comment.isNew()) {
                         Logging.debug("found note change to upload");
-                        processNoteComment(monitor, api, note, comment);
+                        processNoteComment(getProgressMonitor(), api, note, comment);
                     }
                 }
+                getProgressMonitor().worked(1);
             }
         }
 

[taylor.smock]

Improve UI feedback for uploading notes, add basic test to ensure that notes are uploaded when expected. Original patch by gaben, modified. Tests by taylor.smock.

[gaben]

Thank you!

[taylor.smock]

No problem. I'd merge it right now, but I'm still uncertain as to whether or not we are going to do a 22.04 release. Going by wiki:DevelopersGuide/Schedule, it should have been last weekend (last weekend of April), but recent practice has been first weekend of the month (in this case, also last weekend, but we've also been a week or so late, see r18002 (2021-07-11)).

If I haven't merged it in two weeks, I probably forgot about this. Feel free to ping this ticket at that point.

[stoecker]

Milestone renamed

[taylor.smock]

In 18443/josm:

Fix #17288: Notes progress bar shows no progress (patch by gaben, modified)

The patch provided by gaben was modified to only count modified notes for
the progress bar and count. Technically speaking, this is a little less
efficient, as the modified notes will have to iterate through comments
twice instead of once. This should not be noticeable to end users, as
they should not (generally speaking) be closing enough notes for this
to be an issue.

UploadNotesTask now has a test (not by gaben) to check for correctness.