Create Extensibility for Note-Menu

[Kmpopppe]

As per discussion in #22307, taylor.smock asked for a new ticket.

Using the "noteSolver" plugin users can "remember" notes that get closed when they upload the next changeset with both the Note Comments and the Changeset comments referencing each other.

As of right now, a popup menu is used that gets displayed on top of the note when you click it, hiding most of the contents and sometimes appearing in all the wrong places (see #22371).

This ticket asks for an enhancement to make the newly created popup menu, that's displayed in the "Notes" window extensible via the plugin code.

If at all possible, it would be great to have

  final NotesMenu oNotesMenu = MainApplication.getNotesMenu();

work the same as getting the MainMenu object from the application.

It should then either be possible to create a JMenu object with the .addMenu(...) method to the existing NotesMenu OR use .add(...) method to add a JMenuItem object directly to the popup menu.

Kai

[taylor.smock]

Stupid question:

Would it be better to just add a listener to the NoteData collection that fires an event when a note gets changed? i.e.

public class NoteData {
    public void addNoteChangeListener(Consumer<Note> noteChangeListener) { /* Add to listener list */ }
    public void removeNoteChangeListener(Consumer<Note> noteChangeListener) { /* Remove from listener list, useful for plugins that want to be able to update without restart */ }
}

public class NoteListener implements Consumer<Note> {
    @Override
    public void accept(Note changedNote) {
        if (changedNote.getState() == State.CLOSED) { /* Do stuff, like ask the user if they fixed the issue in the current changeset */ }
    }
}

I'm not terribly familiar with the noteSolver plugin and the intended workflow, but it seems like it might be better to listen for changed notes just in case another plugin does stuff with notes (i.e., automatically detects that a specified object has been modified and automatically closes the note).

[Kmpopppe]

Hi taylor and thanks for your comment,

the workflow was supposed to be the user "marking" a note to remember (1..n) that shall be closed (with adding a comment) when the Changeset gets uploaded (I had a hard time detecting the right moment ...).

Using the noteChangeListener sounds good at first glance, yet, the user would always need to do "something" with the note (like adding a comment or marking it as closed) to fire - of course, your argument with another plugin changing anything in the Notes is a good one. I'll have to think about that.

If adding a Menu hook is too much of a stretch (because it is, after all, just one specific use case), I'll go down that Listener route :-)

Kai

[taylor.smock]

Adding the menu hook shouldn't be too much of a stretch. I just want to make certain that I'm fixing the problem in an optimal way.

For

the workflow was supposed to be the user "marking" a note to remember (1..n) that shall be closed (with adding a comment) when the Changeset gets uploaded (I had a hard time detecting the right moment ...).

We probably ought to implement a listener for data upload events and notify listeners of the uploaded changeset. This would probably be a better solution than whatever you are doing.

Using the noteChangeListener sounds good at first glance, yet, the user would always need to do "something" with the note (like adding a comment or marking it as closed) to fire - of course, your argument with another plugin changing anything in the Notes is a good one. I'll have to think about that.

Don't they still have to do that? As in, they have to mark the note as resolved?

[Kmpopppe]

Replying to taylor.smock:

Adding the menu hook shouldn't be too much of a stretch. I just want to make certain that I'm fixing the problem in an optimal way.

Like a good developer does, thanks for taking the time to think about it :)

We probably ought to implement a listener for data upload events and notify listeners of the uploaded changeset. This would probably be a better solution than whatever you are doing.

Today, it looks like this:

        private final DataSetListener dataSetListener = new DataSetListener() {
                @Override
                public void otherDatasetChange(AbstractDatasetChangedEvent event)
                {
                        if (event.getType() == AbstractDatasetChangedEvent.DatasetEventType.CHANGESET_ID_CHANGED && autoUploadDecision) {
                                lastChangeSet = ((ChangesetIdChangedEvent) event).getNewChangesetId();
                        }
                        ProgressMonitor pm = null;
                        if (event.getType() == AbstractDatasetChangedEvent.DatasetEventType.PRIMITIVE_FLAGS_CHANGED && autoUploadDecision && requiresUploadCount == 0 && lastChangeSet > 0) {
                                for (Note note : rememberedNotes) {
                                        String cOnlineStatus = getOnlineNoteStatus(note.getId());
                                        NoteData noteData = new NoteData(java.util.Collections.singleton(note));
                                        if (note.getState() == State.OPEN && cOnlineStatus.toLowerCase().trim().equals("open")) {
                                                try {
                                                        noteData.closeNote(note, I18n.tr("Resolved with changeset {0} by noteSolver_plugin/{1}", getUrl(lastChangeSet, linkTypes.CHANGESET), myPluginInformation.version));
                                                        UploadNotesTask uploadNotesTask = new UploadNotesTask();
                                                        uploadNotesTask.uploadNotes(noteData, pm);
                                                } catch (Exception e) {
                                                        JOptionPane.showMessageDialog(null, I18n.tr("Upload Note exception:\n{0}", e.getMessage()));
                                                }
                                        } else {
                                                JOptionPane.showMessageDialog(null, I18n.tr("Note {0} was already closed outside of JOSM", Long.toString(note.getId())));
                                        }
                                        solvedNotes.add(note);
                                }
                                rememberedNotes = new NoteList();
                                event.getDataset().addChangeSetTag("comment", "");
                                updateMenu();
                                autoUploadDecision = false;
                                lastChangeSet = 0;
                        }
                }

                ... bunch of empty Overrides ...
        };

It would be GREAT if there was a "ChangesetUploadFinished" event, that would supply the CS id and only fire if an upload was successful and not cancelled beforehand. The current implementation does some guesswork and SHOULD only start closing the Notes (the preparation of the Changeset Comment with all the Note Links is done in

private final UploadHook uploadHook = new UploadHook() {
  @Override
  public boolean checkUpload(APIDataSet apiDataSet) {

before the upload starts and that's the easy part ;-))

Don't they still have to do that? As in, they have to mark the note as resolved?

Nope. You use the JPopupMenu when the Note is clicked or from the MainMenu to "remember" the note and once you start uploading the Data changes, you get asked if all remembered Notes shall be automatically closed when you upload the CS - that's what the above mentioned DataChangeListener is doing.

[taylor.smock]

Replying to Kmpopppe:

Nope. You use the JPopupMenu when the Note is clicked or from the MainMenu to "remember" the note and once you start uploading the Data changes, you get asked if all remembered Notes shall be automatically closed when you upload the CS - that's what the above mentioned DataChangeListener is doing.

OK. So a workflow where the user marks a note as closed but does not upload the change doesn't work for you?

Example using your sample code:

                @Override
                public void otherDatasetChange(AbstractDatasetChangedEvent event)
                {
                        if (event.getType() == AbstractDatasetChangedEvent.DatasetEventType.CHANGESET_ID_CHANGED && autoUploadDecision) {
                                lastChangeSet = ((ChangesetIdChangedEvent) event).getNewChangesetId();
                        }
                        ProgressMonitor pm = null;
                        if (event.getType() == AbstractDatasetChangedEvent.DatasetEventType.PRIMITIVE_FLAGS_CHANGED && autoUploadDecision && requiresUploadCount == 0 && lastChangeSet > 0) {
                                // You'll want to check and make certain I got the methods and syntax right -- this was done in the web browser
                                for (Note note : MainApplication.getLayerManager().getLayersOfType(NoteLayer.class).stream().flatMap(noteData -> noteData.getNotes().stream()).collect(Collectors.toList())) {
                                        // Skip remainder if not relevant
                                        if (!note.getLastComment().isNew() && !rememberedNotes.contains(note)) continue;

                                        String cOnlineStatus = getOnlineNoteStatus(note.getId());
                                        // I'm not certain why you aren't just uploading all the changed notes in a single batch, but maybe you have a reason.
                                        NoteData noteData = new NoteData(java.util.Collections.singleton(note));
                                        boolean onlineIsOpen = cOnlineStatus.toLowerCase().trim().equals("open");
                                        if (note.getState() == State.OPEN && onlineIsOpen) {
                                                try {
                                                        noteData.closeNote(note, I18n.tr("Resolved with changeset {0} by noteSolver_plugin/{1}", getUrl(lastChangeSet, linkTypes.CHANGESET), myPluginInformation.version));
                                                        UploadNotesTask uploadNotesTask = new UploadNotesTask();
                                                        uploadNotesTask.uploadNotes(noteData, pm);
                                                } catch (Exception e) {
                                                        JOptionPane.showMessageDialog(null, I18n.tr("Upload Note exception:\n{0}", e.getMessage()));
                                                }
                                        } else if (note.getState() == State.CLOSED && onlineIsOpen && note.getLastComment().isNew() && !note.getLastComment().getText().contains("noteSolver_plugin/")) {
                                                NoteComment lastComment = noteData.getLastComment();
                                                note.getComments().remove(note.getLastComment()); // Remove the immutable comment, although there should probably be a proper API for this instead of abusing the fact that getComments() returns a mutable list
                                                NoteComment newComment = new NoteComment(lastComment.getCommentTimestamp(),
                                                    lastComment.getUser(), lastComment.getText() + "<br>" + tr("Resolved with changeset {0} by noteSolver_plugin/{1}", getUrl(lastChangeSet, linkTypes.CHANGESET), myPluginInformation.version), lastComment.getNoteAction(), lastComment.isNew());
                                                // Do upload stuff.
                                        } else {
                                                JOptionPane.showMessageDialog(null, I18n.tr("Note {0} was already closed outside of JOSM", Long.toString(note.getId())));
                                        }
                                        solvedNotes.add(note);
                                }
                                rememberedNotes = new NoteList();
                                event.getDataset().addChangeSetTag("comment", "");
                                updateMenu();
                                autoUploadDecision = false;
                                lastChangeSet = 0;
                        }
                }

[Kmpopppe]

If this gets to be the new workflow (marking the Note resolved and not uploading it) I will "just" need to tell the users.

I'm not certain why you aren't just uploading all the changed notes in a single batch, but maybe you have a reason.

The sole reason being I didn't even find examples that made a note upload so I had to guess everything together, I didn't even consider that uploading multiple notes at a time was an option :-D

[taylor.smock]

If you have some users you can contact and see what they would prefer, it would be nice.