-
-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Shortcut Editor: Various UIX Tweaks #334
Conversation
Noticed the MP4 videos aren't embedding properly, I'll convert them to webm and reupload. EDIT: Fixed. |
I was just looking into allowing sorting perhaps as part of this PR, but it seems like it isn't that simple. To accomplish this we may have to switch to using What I think I will do is learn a bit more about what ways to go about implementing this kind of sorting, and if there is a better approach than we took for the other TableWidgets in other dialogs, I will open a PR. This shortcuts dialog could act as a testing ground for a better implementation. A possible quick solution would be to set / update a TableItem in the same cell as the LineEdit widgets, but I'd rather look into other approaches for now before implementing anything here. That way, if there is another approach we can use, we can implement it cleanly in this dialog without having to revisit and potentially refactor. It can act as a "clean slate" that won't need as much stripped out to test on locally. So yeah, sorting is missing from this PR, but hopefully more to come in a future PR :-) |
I wonder if there's potential to show the Non-Steam Game AppID here... It's probably not all that useful but maybe we could add it on the tooltip, in brackets on a newline (to avoid getting in the way of the AppName). |
For health reasons I will be taking a break from working on public projects, so I won't be around for a while to review any PR feedback. I can pick this up again when I'm available, or if there is work you or others would like to do that builds on this, feel free to make any necessary changes and merge. 🙂 |
Get well soon and take time for youself! I'll review this long waiting PR in the next weeks and if neccessary do corrections myself. Thanks for the work you put in ProtonUp-Qt and the other projects! |
Overview
This PR makes a few UIX tweaks to the Shortcut Editor dialog:
text
parameter intxt_changed
is a falsey empty string), the tooltip text falls back to the placeholder text.To help illustrate the changes, I made some usage comparison videos against
main
and this PR.main
Branchpupgui2.shortcut.editor.-.main.webm
This PR
pupgui2.shortcut.editor.-.PR.webm
Despite the changes, this should still be compatible with #332.
Implementation
To accomplish the focusing changes I wanted with any amount of flexibility, I created a custom subclass of
QLineEdit
that overrides various the focus events. We track when the line edit is initially focused and use this to know to only select the entire LineEdit text on the first focus click. This allows another click to de-select everything, and avoids everything being selected again on any other mouse click event.One of the things that tripped me up here is the order these events are called, so I'll give some background on that. When you click on the QLineEdit,
focusInEvent
is called. However directly after that,mousePressEvent
is called to process the click. If you try to select all the text in the line edit onfocusInEvent
, the click event processed directly after inmousePressEvent
will override this. But we have to usefocusInEvent
to preserve the selection behaviour on Tabbing into the LineEdit cell. So we have to do two things:To accomplish this, we call the helper method I created on the widget called
focusWithTextSelection
to highlight the text in the LineEdit infocusInEvent
if the focus reason is a tab focus event. This means we ignore mouse focus events, since we shouldn't call this helper method twice. Since we knowmousePressEvent
will handle the mouse click event directly afterwards on a mouse click focus event anyway, we let that method override handle that case. Finally, we use a boolean to track if the focus was already given, which allows us to only select all text on the first focus (meaning a 2nd click won't keep all the text highlighted) and also avoids a case where if we tab into the cell, clicking will move the cursor and de-select all of the text, instead of causing another call tofocusWithTextSelection
.Calling
super
on these methods is pretty important, otherwise you end up with whacky focus/duplicated cursor issues!Motivation
Originally, I just wanted to move the cursor for AppName back to the beginning so that I could see the start of the App Name instead of the end. I eventually fell down a rabbit hole of making some other usability changes that I happened to notice along the way.
Of course all of this is just my own personal tastes and based on my own expectations, so to you and others these behaviours may be unexpected. I'm open to all discussion on any feedback, or on the other hand, other UX changes you think would fit the scope of this PR.
Thanks!