UI: Add bookmark reordering #3757
Conversation
flameflows
force-pushed
the
feature/shift-bookmarks
branch
8 times, most recently
from
1f5f1c2 to
0289e1b
Compare
Signed-off-by: Lisha Wang <[email protected]>
flameflows
force-pushed
the
feature/shift-bookmarks
branch
from
0289e1b to
d98cd18
Compare
Zorro666
left a comment
Zorro666
left a comment
There was a problem hiding this comment.
Thank you for this. Personally occasionally I have wanted to be able to reorder bookmarks and I have usually deleted them and readded them in the order I want which is time consuming and error prone.
Ideally there might be a full UI for reordering the bookmarks using a list with drag'n'drop but that is a fair amount of UI work and this PR is a good balance of not much code and brings useful functionality.
I think this could be built on to easily add actions to move the bookmark to the Start or End as well as Left/Right which would bring a decent set of basic re-ordering for the bookmarks and this fits well with using an enum to describe the movement of the bookmark in the ShiftBookmark method.
Ideally but not essential the context menu would grey out the movement options which are not applicable i.e. Move Left is disabled for the bookmark at the start. One way to achieve that could be by passing in the bookmark index from repopulateBookmarks to bookmarkContextMenu or getting the bookmarks in bookmarkContextMenu to find the position.
| The bookmark associated with the givenevent ID is swapped with the adjacent bookmark in the specified direction. | ||
| :param int EID: The event ID of the bookmark to move. | ||
| :param int direction: The direction to move. |
There was a problem hiding this comment.
I think this would benefit from being an enum instead of an int
The enum could be more expressive
|
|
||
| if(newIndex < 0 || newIndex >= m_Bookmarks.count()) | ||
| return; | ||
|
|
There was a problem hiding this comment.
Be good to early return if "newIndex == index"
| QAction renameBookmark(tr("&Rename"), this); | ||
| QAction deleteBookmark(tr("&Delete"), this); | ||
| QAction moveLeft(tr("Move &Left"), this); | ||
| QAction moveRight(tr("Move &Right"), this); |
There was a problem hiding this comment.
R : keyboard shortcut is already used by Rename : need to choose a different character
|
|
||
| QAction renameBookmark(tr("&Rename"), this); | ||
| QAction deleteBookmark(tr("&Delete"), this); | ||
| QAction moveLeft(tr("Move &Left"), this); |
There was a problem hiding this comment.
would be nice to add an appropriate icon for the new context actions. Perhaps
Icons::arrow_left()
Icons::arrow_right()
| m_BookmarkStripLayout->removeItem(m_BookmarkSpacer); | ||
| m_BookmarkStripLayout->addWidget(but); | ||
| m_BookmarkStripLayout->addItem(m_BookmarkSpacer); | ||
| m_BookmarkButtons[EID] = but; |
There was a problem hiding this comment.
Not sure if this is required ? It is already set on line 5343
| )"); | ||
| virtual void RemoveBookmark(uint32_t eventId) = 0; | ||
|
|
||
| DOCUMENT(R"(Reorders a bookmark by shifting its position in the bookmark list.This allows manual reordering of bookmarks. |
There was a problem hiding this comment.
Looks like some spaces are missing:
"list.This" -> "list. This"
"givenevent" -> "given event"
2 participants
Description
This PR introduces the ability to manually reorder event bookmarks in the Event Browser.
Implementation details: