Added shader viewer bookmarks and find shortcuts. #3312
Conversation
There was a problem hiding this comment.
I think this looks good thanks! I've made one minor code organisation note.
One thing that occurs to me is that this isn't hugely discoverable and it might help to have a toolbar button for this next to 'Find', which people might also find convenient. Clicking the button would toggle a bookmark on the current line just like the context menu does, and possibly you could have a drop-down menu that lists bookmarks in the current file to jump to. With multiple panes visible this gets a little confusing but if the dropdown indicated which file it was listing bookmarks for that would reduce confusion.
That would then make people more likely to find the feature as I'm not sure how often people will be right-clicking for the context menu and shortcuts are quite hidden.
On that note it would be good if you could update the documentation as well to mention this, but if you don't want to I can also do that as a follow up.
qrenderdoc/Windows/ShaderViewer.cpp
Outdated
| { | ||
| m_FindReplace->setReplaceMode(false); | ||
|
|
||
| auto SetFindTextFromCurrentWord = [this]() { |
There was a problem hiding this comment.
I feel this would be better as a plain member function since there's no particular need for it to be a lambda to capture here.
There was a problem hiding this comment.
I made it a lambda only because that code is used twice in the same function.
Is it really worth creating a member function for this?
There was a problem hiding this comment.
I'm not quite sure I'm on the same page - there's no cost to adding a member function so I'm not sure how we'd say it's worth it or not? To me that would be more a question of whether it's worth duplicating the code vs making a function.
I would always reach for a normal function first and go to a lambda if there's a reason it becomes clumsy or awkward, especially since e.g. it's more annoying to step through for debug.
In my view lambdas are good for cases like anonymous callbacks (happens a lot in the UI for invoking between threads), or to grab some local context where it would be clumsy to pass that context in via an additional function parameter. In this case though this is pure code reuse so a member function should be well suited.
There was a problem hiding this comment.
I mean that since this code is only used inside this function and not meant to be part of the ShaderViewer's "API", I figured it would be better to just create a local lambda.
But if you prefer a member function, I have absolutely no problem with that! :)
There was a problem hiding this comment.
Ok I made the lambda a member function.
I think it would be nice to merge this PR into the main branch to at least get the functionality.
I'll work on adding bookmark button (and perhaps a list of bookmarks in a dropdown list like you suggest) in another PR.
Does that make sense?
There was a problem hiding this comment.
I'd rather wait until you're able to make those follow up changes. I don't think there's any rush to get this in so the PR can wait open until you have a chance to add to it and the feature is more well-rounded and complete.
|
I thought about adding buttons, but it was a tiny little bit more involved (need to find proper icons for a start!), so I decided to implement the functionality first, with context menu actions, and I'll add buttons in a separate commit. |
|
That's totally fine, I'll leave this PR for you to add to. For icons you can use these pngs from the same icon pack as the others: bookmark_red.png bookmark_red@2x.png |
|
Would these icons be ok for Toggle/Prev/Next Bookmarks? Or maybe this one for Toggle Bookmark? |
|
Yeh I saw that last one for toggle bookmark but I thought the simple bookmark was clearer. I wasn't thinking you'd need a button for previous/next as the main idea would be to let people see that a bookmarking feature exists at all - similar to find previous/next that can be left to context menus or shortcuts. |
|
Added a bookmark menu: I created the blue bookmark icon myself and combined it with some existing icons. |
There was a problem hiding this comment.
These files should be suffixed @2x not @x2 as with the other icon files. Also I should have been clearer in my earlier comment sorry - please use the icons that I shared before:
I appreciate that you've done a good job making one yourself, but I'm not an artist and the easiest way to have visual consistency is to use pre-existing icons from that icon pack.
Also to say here since it's related, you can probably omit the icons for next/previous/clear. Having an icon for the toolbar button itself is important but it's fine to have just text in the drop-down entries with that context. In this case the additions are a bit hard to read at normal size of 16x16.
If you feel strongly about keeping icons for them you could use the existing previous/next/delete icons without any bookmark-specific thing. Since these icons aren't used anywhere else the context is very clear. If you want to keep the bookmark icon as well as the arrow it might be possible to make it work, but I think it would need to use a different arrow at least.
There was a problem hiding this comment.
Sorry for the bad naming...
I just didn't like the fact that the icon was red. For visual consistency, a blue icon fits better with the blue markers.
I don't want to use red for the Scintilla markers because the breakpoint markers are already using than color.
I still feel like the icon I created is clear and fits with the other ones. I know the smaller icons with arrows are a bit fuzzy (i started with 32x32 images and just scaled them down), but there's text beside them so it's clear enough imo.
But if you really prefer to stick to the icon set, and you don't mind the different color, I can use the red icon + arrow icons in the menu. Just let me know!
There was a problem hiding this comment.
Here, I used my own bookmark image with an existing 16x16 arrow icon from the standard icon set:
There was a problem hiding this comment.
I'm not worried about the colour myself, but that's a reasonable point. Here's the icon from the icon pack with the red and blue channels swapped: 
I have concerns about the arrows but I don't think your base icon is unclear or unfitting, the reason I'm asking you to stick to the icon pack as a safer option is that I am aware that I'm not an artist or visual designer and I can't really judge accurately. The problem with showing 32x32 icons for comparison is that they will virtually never be seen, the 16x16 icons are what almost everyone will see so they should be readable, scaling down at these levels is not a good option as icons of this size are more-or-less pixel art. Again I agree that making an icon for something like 'next bookmark' is challenging in the space, it would ideally need something specifically drawn for it.
To be clear as I mentioned above it's definitely not a requirement at all to have an icon for every menu item - my preference and if I were implementing this feature then the icon would only be used for the toolbar button and the menu items would be just text. If you want to use the existing arrow_left/arrow_right/del then I'm fine with that, but please use the blue-flipped icons above for the bookmark button.
There was a problem hiding this comment.
Swapping the channels is a good idea! 😁
Can I use the one you made or perhaps slightly alter the color to match the one I used for the markers?
There was a problem hiding this comment.
Yes please use the icons from the comment above, the colours don't have to exactly match so it's fine like that.
| bookmarkMenu->addAction(act); | ||
|
|
||
| ui->bookmark->setMenu(bookmarkMenu); | ||
| } |
There was a problem hiding this comment.
I think here we should mark the previous/next/clear buttons as disabled when there are no bookmarks in the current file as a typical UI convention.
You can do this with the QMenu::aboutToShow slot on the menu, if you keep the actions as separate named variables and capture them in a lambda then conditionally call setEnabled.
There was a problem hiding this comment.
Oh yeah I'll do that! I did it for the context menu but forgot about this one!
qrenderdoc/Windows/ShaderViewer.ui
Outdated
| </property> | ||
| <property name="icon"> | ||
| <iconset resource="../Resources/resources.qrc"> | ||
| <normaloff>:/bookmark_blue@x2.png</normaloff>:/bookmark_blue@x2.png</iconset> |
There was a problem hiding this comment.
The icon referenced by default should be the normal version, the @2x versions will be automatically substituted by Qt if the dpi ratio is high enough.
There was a problem hiding this comment.
I can't believe I misnamed those... Time to change my glasses!
|
Now using icons from standard icon set.
|
qrenderdoc/Windows/ShaderViewer.ui
Outdated
| <item> | ||
| <widget class="QToolButton" name="bookmark"> | ||
| <property name="toolTip"> | ||
| <string>Run or step backwards in execution</string> |
There was a problem hiding this comment.
Sorry, I noticed this last time and I would have sworn that I put a comment on it, but I must have been mistaken. This tooltip is not correct for the button.
There was a problem hiding this comment.
Ha! That's because I copy-pasted the button!
I'll fix it no problem!
There was a problem hiding this comment.
Would this be ok?
<property name="toolTip">
<string>Add, remove or jump to bookmarks in current file</string>
</property>
Added shader viewer bookmarks and find shortcuts.