frontend: Set cursor width for OBSHotkeyEdit to 0#12466
Conversation
9af0480 to
3315ec3
Compare
PatTheMav
left a comment
There was a problem hiding this comment.
Wouldn't it be better to create a bespoke proxy style for the hotkey edit widget instead of having to import the header and add a dynamic_cast runtime check to figure out if the widget in question is based on OBSHotkeyEdit?
Because this code as-is will run for every widget and has to do this check for every widget and will have a non-zero return in a very small percentage of its overall calls. And that seems wasteful to me.
3315ec3 to
9ad145c
Compare
|
I've swapped the comparison around, that was indeed quite wasteful (now it should only be The overhead now is pretty much only this one function call, personally I didn't consider that to be huge issue and had intentionally opted against the per-widget style (to keep down code complexity). But if you want me to change that I can also do that of course. |
|
Refresh my memory please - how does one enable a proxy style? A quick search gave me |
Yes. Additionally, a style (including a proxy style) could be set on a widget, that style would then be used instead. |
|
Got it, yeah it still rubs me the wrong way that this change has to include the header for a specific widget and still do dynamic casting checks (which would still take place for any widget that will use a text cursor) for all widgets in the entire app... ...just for the widgets used on a single page of the Settings window. |
|
I'll change it to only be set on the specific widgets then. |
Much appreciated, because the code looked fine otherwise. |
9ad145c to
12a5941
Compare
|
Updated this PR to have a separate |
PatTheMav
left a comment
There was a problem hiding this comment.
Wouldn't it make more sense to either generate a single instance of each available style once and have the instances live as public properties of the singleton OBSApp instance or add a static factory function to the proxy styles themselves that returns the a singleton instance?
Singleton patterns are ugly, but in cases like these represent the reality of the running application. And at the end of the day each hotkey input widget will just use that instance to call setStyle as far as I can tell.
I dunno what the style guides for Qt are about this, it's already a bit mad that the proxy style has to be a Q_OBJECT instead of a simple static function.
|
Because I just spotted the remark on #12465, I'm actually not sure whether this is the right way to do it, at least on macOS: Shortcut input fields always have a text cursor on macOS, because it's consistent for "input fields". |
well the guidance from Qt is to not use
That's why this definitely needs @Warchamp7's opinion. Not having the cursor would at least be consistent with Qt's own |
Ok, that's a separate decision to make then. That leaves my code-style concerns over having to pass the style instance all over the place from my original comment. |
12a5941 to
294fa59
Compare
Updated to have only one instance in OBSApp. |
PatTheMav
left a comment
There was a problem hiding this comment.
I had further ideas, but enough is enough.. 😂
|
This has a merge conflict due to #12463. |
294fa59 to
686834c
Compare
|
For what it's worth, on Windows in OBS Studio 30.1.2 and 31.1.2, the hotkey edit field has a blinking cursor. |
This hides the cursor from the OBSHotkeyEdit spin boxes even when they are in focus. Uses a shared style owned by OBSApp.
686834c to
9ee51f5
Compare
Description
Removes the blinking cursor from hotkey edit fields.
While related to #12465, it doesn't depend on it (or the other way around) and they can be merged separately.
Motivation and Context
Up until OBS 31.0(?), the cursor was hidden on macOS.
This was a conscious decision back then (the removal in #12465 is due to theming changes making that particular method impossible). It's unclear why it was only done on macOS, perhaps this used to be the default on Windows already.
Removing the cursor mimics Qt's own
QKeySequenceEdit, which also doesn't have a blinking cursor.As also noted in the comment removed in #12465, the focus should already be clear. To me personally, having a cursor suggests that the keys pressed are appended instead of replacing the current ones.
How Has This Been Tested?
macOS 26
The cursor is gone now. (tested in combination with #12465 as without it, the cursor would be gone on macOS anyways)
Types of changes
Checklist: