Modify

Opened 5 years ago

Closed 5 years ago

#12409 closed enhancement (fixed)

[patch] arrow on edit relation button lists recent relations

Reported by: kolesar Owned by: kolesar
Priority: normal Milestone: 16.02
Component: Core Version:
Keywords: Cc:

Description

Added a button to relation list dialog for opening last edited relation. It is useful when you have closed the dialog, changed selection and new selection is not member of the relation.

Last relation is stored in properties of data layer. Button is enabled/disabled on layer change. Action has an empty keyboard shortcut customizable by user.

Attachments (6)

EditLastRelation.patch (8.4 KB) - added by kolesar 5 years ago.
RecentRelationsAction.patch (13.4 KB) - added by kolesar 5 years ago.
latestrelationslist.png (20.7 KB) - added by Klumbumbus 5 years ago.
IconForRecentRelations.patch (1.0 KB) - added by kolesar 5 years ago.
IconSizeNotHardcoded.patch (1.9 KB) - added by kolesar 5 years ago.
RecentRelationsHideArrow.patch (638 bytes) - added by kolesar 5 years ago.

Download all attachments as: .zip

Change History (29)

Changed 5 years ago by kolesar

Attachment: EditLastRelation.patch added

comment:1 Changed 5 years ago by bastiK

I understand the problem and where you are coming from. But my feeling is that this is too special and not the right place in the GUI. In addition it is not immediately clear what "last relation" refers to: Last selected - no; last clicked in the relation list - no. Last one opened in relation editor - bingo!

What I could imagine is a drop down button like in selection editor which lists the last couple of edited relations.

comment:2 in reply to:  1 Changed 5 years ago by kolesar

Replying to bastiK:

I understand the problem and where you are coming from. But my feeling is that this is too special and not the right place in the GUI. In addition it is not immediately clear what "last relation" refers to: Last selected - no; last clicked in the relation list - no. Last one opened in relation editor - bingo!

What I could imagine is a drop down button like in selection editor which lists the last couple of edited relations.

This action opens the relation that was closed last time. It works as an undo for closing relation editor. Similar to History / Recently closed windows / <first entry> Ctrl+Shift+T in web browsers.

I did not want to complicate existing Edit button and popping up last closed seemed to be a good trade-off. I will update this patch to remember a list of recently closed relations and shortcut will pop up first entry. This way shortcut can be pressed repeatedly, similar to browser tabs.

comment:3 Changed 5 years ago by stoecker

Still not a good design. Josm has these small arrows (e.g. on search button) for such purposes. Don't introduce new methods for similar purposes.

comment:4 in reply to:  3 Changed 5 years ago by kolesar

Replying to stoecker:

Still not a good design. Josm has these small arrows (e.g. on search button) for such purposes. Don't introduce new methods for similar purposes.

I meant (but have not written explicitly) to add small arrow to open dropdown list to edit button, similar to search button. It means I will drop additional button introduced in the first version. Is it OK?

comment:5 Changed 5 years ago by bastiK

Yes, this is good.

comment:6 in reply to:  5 Changed 5 years ago by kolesar

Replying to bastiK:

Yes, this is good.

I have rewritten feature. Renamed to RecentRelationsAction, manages a list of recent relations. Placed a small arrow to edit relation button. Arrow opens a popup menu with recent relations. Arrow is disabled when popup list would be empty. Checks for deleted relations, undo/redo operations and layer changes.

Most recent can be opened with keyboard shortcut Shift+Escape that is currently available. Shortcut key is displayed in list when assigned.

Opening relation editor removes entry from list, closing dialog adds to top of list. CreateMultipolygonAction also adds new relation to list, this is useful for quick editing.

Changed 5 years ago by kolesar

Attachment: RecentRelationsAction.patch added

comment:7 Changed 5 years ago by kolesar

Owner: changed from team to kolesar
Summary: [patch] edit last relation[patch] arrow on edit relation button lists recent relations

(I have changed owner but I do not have commit privilege yet. I ask someone from the team to review and commit.)

Last edited 5 years ago by kolesar (previous) (diff)

comment:8 Changed 5 years ago by stoecker

Milestone: 16.02

comment:9 Changed 5 years ago by bastiK

Resolution: fixed
Status: newclosed

In 9668/josm:

applied #12409 - arrow on edit relation button lists recent relations (patch by kolesar)

comment:10 Changed 5 years ago by Don-vip

thanks for the patch, but for future ones please check that:

  • no checkstyle violation is added
  • no public API without javadoc is added
  • no invalid javadoc is added (missing params for example)

see r9674, r9676, r9678

Last edited 5 years ago by Don-vip (previous) (diff)

comment:11 in reply to:  10 Changed 5 years ago by kolesar

Replying to Don-vip:

thanks for the patch, but for future ones please check that:

  • no checkstyle violation is added
  • no public API without javadoc is added
  • no invalid javadoc is added (missing params for example)

see r9674, r9676, r9678

Sorry for my early mistakes. I'm just warming into Java development.

Are the validation tools documented in Development Guidelines? I'm afraid not. Javadoc is mentioned but way of generating/checking is not. I have extended DevelopersGuide/StyleGuide with automated checks, please verify them.

I have not found a way to run findbugs on specific files from the command line, only from ant that processes all files. This takes a very long time and produces a 7000+ lines .xml file. It is hard to find bugs related to my changes.

I'm still using plain text editor for editing and ant for build because I was unable to set up Eclipse workspace correctly. Added project, build starts, finds a lot of errors like this:

Duplicate methods named spliterator with the parameters () and () are inherited from the types Collection<B> and Iterable<B>

Can you help me?

comment:12 Changed 5 years ago by stoecker

It's not that we expect patches to be 100% correct. Vincent's comment requests that you try to be clean, not that your always reach that goal. Most of the rules are pretty obvious, so you can follow them also without the checking tools.

If you look at the automated builds you see that we have about 249 FindBugs entries, 21 of these with high priority. 240 Coverity issues with 19 high impacts (thought Coverity hangs with the update, some of these should already be fixed). But we are clean for style and Javadoc checks.

Goal is to get rid of these issues. Ultimate solution to find new issues easier in the log is to fix all outstanding ones :-)

Changed 5 years ago by Klumbumbus

Attachment: latestrelationslist.png added

comment:13 Changed 5 years ago by Klumbumbus

Resolution: fixed
Status: closedreopened


There is an empty space before the text (position of mouse pointer). Whats the reason for this? Could we display the icon there (in this case the river icon)?

comment:14 in reply to:  13 Changed 5 years ago by kolesar

Replying to Klumbumbus:

There is an empty space before the text (position of mouse pointer). Whats the reason for this? Could we display the icon there (in this case the river icon)?

On Linux (GTK+) there was no empty space before the text. Thank you for the idea, implemented in a single line of code.

Changed 5 years ago by kolesar

comment:15 Changed 5 years ago by bastiK

Resolution: fixed
Status: reopenedclosed

In 9700/josm:

applied #12409 - icon for recent relations list (patch by kolesar)

comment:16 Changed 5 years ago by stoecker

Resolution: fixed
Status: closedreopened

Don't use hardcoded icon sizes. ImageProvider has function to get size information.

Changed 5 years ago by kolesar

Attachment: IconSizeNotHardcoded.patch added

comment:17 Changed 5 years ago by kolesar

I have just copied code from OsmPrimitivRenderer. Attached patch patch calls ImageProvider for size.

comment:18 Changed 5 years ago by stoecker

If there are other places with hardcoded image sizes, they need to be fixed as well...

comment:19 Changed 5 years ago by simon04

Resolution: fixed
Status: reopenedclosed

In 9705/josm:

fix #12409 - Refactor ImageProvider.ImageSizes

comment:20 Changed 5 years ago by kolesar

I was also working on the same topic, you were faster. Thank you for refactoring.

comment:21 Changed 5 years ago by stoecker

Please remember with the HIDPI the mapping will be no longer direct, but probably factor*size. I wanted to implement this in getImageSizes() after the switch. Now it probably would go into getImageSize() and getImageDimension(). But keep this future adaption in mind when changing more.

comment:22 Changed 5 years ago by kolesar

Resolution: fixed
Status: closedreopened

Modified code to hide arrow when there are no recent relations. Previously arrow was disabled that was rendered as a gray rectangle on GTK+ look&feel. When right bar was set to narrow, this small rectangle partly covered edit icon.

Changed 5 years ago by kolesar

comment:23 Changed 5 years ago by bastiK

Resolution: fixed
Status: reopenedclosed

In 9722/josm:

applied #12409 - arrow on edit relation button lists recent relations: fix (patch by kolesar)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain kolesar.
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.