- May 23, 2021
-
-
-
with an earlier commit in this set having added a tooltip using longtext to the first column of the hotkey editing table, allowing users to get an additional description to help them understand the purpose of an action, it would be great for best UX if we have such a tooltip for each and every entry in the table. of the 113 entries, 9 were missing longtext and thus missing the tooltip. it was for this reason i skipped hotkeys in d75459a3. the "Select the hotkey to use..." form was used with that being fairly common for hotkey longtext. i used a single common longtext for the four zoom presets, as done already with bookmarks and subtitle text scaling, reducing translator burden.
-
these are not for hotkey options they are for playlist options
-
move the clear playlist and subtitle scaling hotkey options above the bookmarks and jump-size ones. - in the hotkey widget they are surely more interesting than the big block of bookmark ones that otherwise come at the end. - in help output, since the bookmark ones and jump sizes ones each declare sections, these options appear miscategorised as being a part of the bookmark section. this was overlooked in 711733c6, or more likely perhaps lost from it in a rebase over time.
-
-
-
-
the parent is always the table.
-
we need 4 columns, not five. one element on the line with four was being pointlessly stretched across two.
-
now the above non-global check has been reduced to a single name check, we can strip the reverse of that here with an else. the one remaining condition is just to avoid work for empty mappings.
-
unlikely and better to add it rather than skip. being there with a blank label would better help diagnose what went wrong in testing, or if it slipped past testing, someone is more likely to notice and report the problem leading to a fix. note that we must conditionally use `qfut()` because the underlying translation function does not like nulls, and my fix for that was rejected.
-
we now rely upon checking option name rather than shorttext. it is somewhat less likely that a mistake with duplicate option names will have been made compared to duplicate shorttext, and duplicate option names presents an even bigger and broader problem that we currently do nothing about. this is not very useful towards helping with that, we'd want a full check of the entire table's option names if we cared to properly check things here.
-
this avoids doing a translation lookup of the displayed shorttext, and using the option name just feels more "correct". note that we strip the "global-" prefix with `+7` to get the match.
-
rather than getting a list of matching items, instead iterate over the list and compare, thus avoiding the allocations in building that list. this also puts us in a position to locate the row by option name rather than shorttext, which might be better.
-
the "Unset" text used to be used for all unset cases, but a long time ago all except this one got changed to use an empty string instead. having spent a bit of time trying to track down an answer in the git history as to the question of why, i gave up. i found no clear answer in the time spent looking. it very well may have been simply overlooked rather than deliberately left to create a clear distinction that a failure occurred. i'm going to assume that it was overlooked. i've thus changed it to return an empty string. i don't expect this to cause any problems for the two different uses in the Qt code.
-
-
the displayed text is always displayed translated for non-global, and this is the case for edits for global, but the initial unedited displayed text was not translated. due to the way the global data is temporarily stored in a `QMap`, it seems that the easiest way to fix this is to simply convert from `QString` to raw string pointer with `qtu()` and then back to `QString` with translation applied with `qfut()`.
-
-
let's start the text with something describing the actual purpose of the overall control, rather than leaping directly into instructions on how to use it. this may not be so important for where the table is shown in the simple view, but makes a notable difference to use in the advanced view, where it looks very odd without it. also, let's clarify those instructions a bit.
-
a tiny touch of padding makes a world of difference to the look of the table imo. notes: - originally i had specified `-1` for width, signalling default, which worked perfectly. however now when testing before publishing this work, i found the change was no longer working. however it does work when specifying `0` for width instead. it seems behaviour changed or a bug was introduced in newer Qt versions. - i tried an alternative solution of using `sizeHint()` on the first element to get the initial dimentions, then increasing the height by 12px before applying that, however this does not work since values of `-1` are returned from `sizeHint()`. there does not seem to be a way of getting the actual size to increase it, or to set an amount of margin/padding to apply.
-
this makes longtext of hotkey actions available to users, helping them to understand the purpose of the actions listed in the table. this does not interfere at all with the tooltip giving instructions on how to perform editing, that still appears when hovering over the other two columns. no tooltip existed for the first column previously.
-
the old solution: 1) collected a list of all matching items. this could include duplicates when searching all columns. 2) iterated over all of the items, for each one checking whether or not it was contained within that set of matches. the new solution simply iterates over the items, checking the one or more columns and setting the hidden property as appropriate as it does so. -
i.e. update when user changes field selection combobox. the filter function is changed here to fetch the filter text directly from the textbox to support this. note that this uses Qt's `connect()` directly, bypassing the vlc `CONNECT()` macro. doing otherwise, it compiles but does not work.
-
keep the existing separator (tab) for stored values, but swap with comma+space separator for display. it works much better this way imo and is much less confusing. it also happens to match the style used by the `QKeySequenceEdit` control. we thus avoid the problems that the default multi-key bindings create, as is the default on Linux currently, whereby with play/pause for example you see "Space Media Play Pause" which obviously looks odd. now you will see "Space, Media Play Pause". if users assign the actual comma key in a multi-key binding, it should not really be a problem for them to grasp what's what. for instance ",, Media Play Pause".
-
the use of multi-key bindings (default) on some platforms, like mine, makes the small starting width of the column problematic. this helps avoid users having to resize the column manually to see everything.
-
the hotkey conflict check did not understand actions which have more than one key/key-combination (tab separated) assigned, and as such it allowed keys/key-combinations to be assigned to more than one action in some cases. e.g. on Linux systems, the "key-play-pause" action has both "Space" and "Media Play Pause" keys assigned, but because the string is "Space\tMedia Play Pause" not "Space" and because the conflict check just matches against the entire string, the conflict is overlooked if trying to set "Space" on something else. conflict checking became broken by default for non-MacOS systems back in v2.1 due to baf3e50b introducing default multi-key bindings, without testing the hotkey editor properly. on Windows, thanks to e8668774, it happened to become unbroken by default for v2.2. note that the editor currently does not support users assigning multi-key bindings; so the problem only comes up when multiple bindings are the default, or if users force them by manually editing their config file. note also that this obviously only fixes Qt; the problem likely still exists in the MacOS code (which I leave for a mac guy to handle), but thankfully there are no such dual bindings there by default, so users won't notice (unless they've manually modified their config file).
-
in the 'selected' text, inconsistent with that done for keyboard events.
-
we had: 1. event making translated copy for 'selected' text. 2. conflict check making an untranslated copy (one commit back in this set it was another translated copy). 3. the 'accepted' code, if triggered, making a further one of each. a single copy of each can be created and stored in public attributes by the event handlers, and used in all of these places, without any negative impact. this replaces the need for the public `keyValue` attribute, and replaces the need to give the conflict checker a copy of the integer. -
with the previous commits having put us in the position to do this, let's adjust the conflict checker to avoid comparing translated text, which is entirely unnecessary. condition split into two for neatness due to line length issue.
-
this replaces fetching a list of conflicts to react to, with iterating over the table and looking for conflicts as we do so. the primary purpose of this is not efficiency, though it is more efficient; it is actually to support the 'multi-key assignment conflict check' fix to come in a subsequent commit in this set. furthermore it is needed to allow improvement of the conflict checker regarding avoiding translation (we need to be able to compare the custom data attributes for that, not the displayed text, as `findItems()` does).
-
if you tried to set a hotkey to use the same key as it already was set to, it would detect a conflict in that the selected key was already in use, failing to recognise though that it was conflicting with itself, thus resulting in a pointless and silly warning.
-
rather than taking just the action name of the to-be edited item. this is necessary for the fix in the next commit.
-
firstly, note that text displayed in the table is (supposed to be) translated (it is not currently it seems for the global column), whilst each key mapping cell holds a data attribute containing an untranslated copy of the actual mapping value text. the logic here: 1. asked `VLCKeyToString( i_vlckey, true )` to convert the captured key/key-combination integer to string form (with translation). 2. searched the display text of the relevant table column to find entries that matched. 3. looking only at the first match (conflict), if there is one, adjusted the dialog for displaying the conflict condition. if conversion failure occurs in step 1, the conversion function returns "Unset". the behaviour then is such that we allow storing this text for the item, thanks to step 3 ignoring conflicts with entries that have an unset state (whether "Unset" or empty). the change made here concerns the complexity and inefficiency of how the conflict check is skipped for such a situation, and the legitimacy of the is-empty side of it. so firstly, wrt. the former: if the data value of the inspected conflict item is "Unset", then this must be because the corresponding display text was "Unset" (the translated form), and thus that the search text (the converted key) was same. (we can safely ignore the hypothetical case of translation resulting in the same text for both "Unset" and some actual key name as being unrealistic, and that will be of no concern when we switch to using the non-translated text for conflict checking in a later commit in this set anyway). given this, we can simplify and optimise handling this situation by checking if step 1 returned "Unset" up front, and directly accepting it if so. this is a much more simple check, and saves effort. (note that we no longer check "Unset" against the "existing keys" set, but we would never expect a legit conflict there anyway). secondly, the situation of the data value being empty i believe is bogus. an item's data value should only be empty where it is in a "proper" unset state (not the "Unset" text situation just mentioned), which can only occur either through the saved value being so; from use of the `del` key; or from using the `unset` button in the edit dialog. in all three cases the display text should also be empty. such an item could thus only be in the conflict search results if the search text itself had been an empty string. since an empty string does not represent any key/combination, and since the int to string conversion always gives "Unset" upon failure, it should not be possible for an empty string search to take place, and thus we would never encounter a conflict item with an empty data value. we can thus ditch the check for that. -
the `locale` boolean param determines whether or not we are asking for a translation to be performed. in the "unset" (error?) case, translation was incorrectly always being performed upon the string "Unset", which meant that any caller not working with translations and wanting to check for the "Unset" condition would fail to work correctly in situations where the translation would result in something other than exactly "Unset". if translation has not been requested, return the untranslated form. this means that now `KeySelectorControl::selectKey()` when reacting to `QDialog::Accepted` will correctly store the untranslated form of "Unset" where applicable in the data attribute. (the data attribute is supposed to always hold an untranslated copy). this also means that the data attribute comparison within the `KeyInputDialog::checkForConflicts()` function will now correctly work in translation situations. note: it is unclear why we return "Unset" here rather than an empty string, creating a distinction from all other unset actions. git history shows that there was a shift from using the text "Unset" to using an empty string, yet this was left, and without any clear explanation as to why, with a very real possibility that it was simply overlooked. here we just leave that question alone, to return to later.
-
if the edit dialog detects a conflict, the 'conflicts' attribute is set to true and the dialog updated to warn the user and give them a choice as to what to do about it. the point of the 'conflicts' attribute relates to one of the choices given to the user, which allows forcing the assignment. in that case, this flag lets `KeySelectorControl::selectKey()` know that it must take action to remove the assignment from existing keys after the dialog closes, before storing the new assignment. another choice available to the user is that of simply entering a different key/combination. if the user does so, this will trigger another conflict check. this new conflict check could obviously again find a conflict, in which case the dialog is updated to reflect the new conflict info. if it does not find a conflict for this new assignment, it is accepted. in this latter situation, of an initial conflict, which the user resolves by making a different choice which then does not conflict, the 'conflicts' flag was left set to true from the original conflict, which meant that the above mentioned function would waste time trying to remove the existing assignment before storing the change. this fixes that inefficiency by simply resetting the flag at the start of performing a conflict check.
-
this widget is that through which all hotkeys are edited. as just explained in the previous commit, the item pointer provided to the constructor is always that of a single hotkey config item. the point in passing along an item pointer was that the base control class requires one. note that all other controls target a single config item and so it makes sense for them, but does not make any sense for this. for whatever reason, someone thought it made more sense to pass along an arbitrary hotkey item pointer rather than null (possibly in relation to the fact of displaying the incorrect tooltip removed in the previous commit). since the param is entirely useless for this widget, this removes it and passes along null to the base control constructor instead. this control simply does not need to be linked to an individual item. note that the line removed involving "key-play" is deliberate, that was the arbirarily chosen item to use for the widget in the simple view. (the `p_config` var is still needed for other things though).
-
the item pointer provided to the constructor is that of a single hotkey config item. using the longtext from this item in a tooltip on the label for the entire hotkey table makes zero sense, so let's remove it. - for the table shown in the simple view, the "key-play" option was arbitrarily used for this. - for the table shown in the advanced view, the text from the first hotkey config item encountered was used.
-
left over from a time long ago when int values were stored in the item data attributes rather than string values. fwiw, just to prove it's redundant, a quick review of history: - first, 4c90cbe8 should have removed it, but it was overlooked. - second, 18049f26 replaced the global instance only with a string !empty check, mistakenly breaking the ability to save unset global hotkey assignments (#7932). - thirdly, 9d7bf49b fixed #7932 by removing the bogus !empty string check for the global instance, but missed the opportunity to remove the old useless check for the non-global instance.
-
this allows us to tidy up the conflict checker code a little, and make use of the column index enum without creating a mess.
-
-
