| rkflx |
| Gwenview |
On Start Page → Recent Files the context menu is missing and
right clicking on an item switches to View Mode.
This patch adds the context menu to Recent Files tab items
with the entries Add Containing Folder to Places,
Forget This File and Forget All Files. The entries on
the Recent Folders tab are renamed accordingly.
Ref T8194
FIXED-IN: 18.04.0
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
Maybe it would be cleaner to handle the context menu in HistoryThumbnailViewHelper like e.g. ThumbnailViewHelper does. But this needs some bigger changes to keep track of the selected item. This is done by ContextManager elsewhere.
| app/startmainpage.cpp | ||
|---|---|---|
| 310 | Without a trailing slash this works for folders too and returns a short name as bookmark label. | |
| app/startmainpage.ui | ||
| 49 | Needed for checking on which tab the context menu is displayed. | |
Awesome, works better than in KDE4 (where the file previews were broken… ;)
However, I don't think there should be an Add File to Places entry. Perhaps in some very rare cases it would make sense to "bookmark" a particular image, but the Places panel is actually more about folders and also in sync with Dolphin and every file dialog (unless you toggle the corresponding checkbox). Clicking in Dolphin on a "Places" image works not that well, and also in Dolphin you are only allowed to add folders to the sidebar, but not files.
Here is an alternative idea: Change this to Add Containing Folder to Places.
(Will continue with the actual code review once this has been changed.)
Maybe it would be cleaner to handle the context menu in HistoryThumbnailViewHelper like e.g. ThumbnailViewHelper does. But this needs some bigger changes to keep track of the selected item. This is done by ContextManager elsewhere.
Ah, you mean ThumbnailViewHelper operates on currentUrl, while the selected item in HistoryThumbnailViewHelper is different from that? Maybe we could just reuse currentUrl, because visiting the Start Page invalidates this variable anyway?
| app/startmainpage.cpp | ||
|---|---|---|
| 283 | I would use title case here (and also at the other place), i.e. "This" instead of "this". | |
| 310 | Haha, it did the same thing in my patch (which I cleaned up on the weekend for the Beta, but it became too late to be able to still post it in the evening). We'll figure out the merge conflict when we know which patch will go in first. | |
I agree - that makes more sense. :)
Ah, you mean ThumbnailViewHelper operates on currentUrl, while the selected item in HistoryThumbnailViewHelper is different from that? Maybe we could just reuse currentUrl, because visiting the Start Page invalidates this variable anyway?
ThumbnailViewHelper doesn't know anything about the selected or any other item - it just displays the menu with actions. The item to operate on is identified in the corresponding slot.
FIXED-IN: 18.04.0
@rkflx Sorry, I don't want to rush - if it doesn't make it for this version it's absolutely ok for me! ;)
Some parts were moved around and some changes weren't directly related to the topic which made the patch a bit harder to follow, but in the end all changes make sense to me, so I'm not complaining too much ;)
Patch LGTM. When landing, watch out for my conflicting change…
Maybe it would be cleaner to handle the context menu in HistoryThumbnailViewHelper like e.g. ThumbnailViewHelper does. But this needs some bigger changes to keep track of the selected item. This is done by ContextManager elsewhere.
Looked at this again, it seems HistoryThumbnailViewHelper is quite unfinished, not sure if it's doing anything actually useful currently. I guess it's an opportunity for future cleanup and adding of functionality (e.g. right now the mouse cursor signals acceptance for drops, but after dropping nothing happens).
| app/mainwindow.cpp | ||
|---|---|---|
| 345–350 | It does not make that much of a difference here, but instead of a copy capture-default ([=]) you could just capture this. | |