DavidoTek / ProtonUp-Qt

Install and manage GE-Proton, Luxtorpeda & more for Steam and Wine-GE & more for Lutris with this graphical user interface.
https://davidotek.github.io/protonup-qt
GNU General Public License v3.0
1.16k stars 39 forks source link

Shortcut Editor: Various UIX Tweaks #334

Closed sonic2kk closed 5 months ago

sonic2kk commented 6 months ago

Overview

This PR makes a few UIX tweaks to the Shortcut Editor dialog:

To help illustrate the changes, I made some usage comparison videos against main and this PR.

main Branch

pupgui2 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 on focusInEvent, the click event processed directly after in mousePressEvent will override this. But we have to use focusInEvent to preserve the selection behaviour on Tabbing into the LineEdit cell. So we have to do two things:

  1. Highlight all text in the LineEdit on the first mouse leftclick focus event
  2. Highlight all text on the Tab focus event (this can only happen once, since to tab into the cell again, you have to tab out and then back in; you can't use tab to focus the same cell twice without first switching cells)

To accomplish this, we call the helper method I created on the widget called focusWithTextSelection to highlight the text in the LineEdit in focusInEvent 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 know mousePressEvent 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 to focusWithTextSelection.

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!

sonic2kk commented 6 months ago

Noticed the MP4 videos aren't embedding properly, I'll convert them to webm and reupload.

EDIT: Fixed.

sonic2kk commented 6 months ago

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 QTableListItems like we did in #137, though my searching led me to something called QTableListView which is apparently better when dealing with cells that contain one (or more) widgets. I really don't have any idea how that all works right now, but I wanted to call out that I'm aware sorting isn't handled in this PR.

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 :-)

sonic2kk commented 6 months ago

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).

sonic2kk commented 5 months ago

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. :slightly_smiling_face:

DavidoTek commented 5 months ago

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!