Enhance: remove minuscule text from under profile icons by SlySven · Pull Request #4712 · Mudlet/Mudlet

SlySven · 2021-01-29T11:16:46Z

This finally enables us to do away with the text under the profile icons as it replicates the normal QListWidget behaviour to move the selection to each item which begins with the first letter of the key pressed.

Signed-off-by: Stephen Lyons slysven@virginmedia.com

This finally enables us to do away with the text under the profile icons as it replicates the normal `QListWidget` behaviour to move the selection to each item which begins with the first letter of the key pressed. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

add-deployment-links · 2021-01-29T11:16:48Z

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

vadi2

This does work, but not quite as good as the Qt version - in there if I have multiple profiles starting with a similar name, I can type the name of the profile and it'll find it. Here, only the last letter is considered, and while I can press the letter many times - it's not as intuitive as entering in a few first letters of the profile.

Do you think you could have this match the behaviour?

SlySven · 2021-01-29T19:16:26Z

This does work, but not quite as good as the Qt version - in there if I have multiple profiles starting with a similar name, I can type the name of the profile and it'll find it.

Oh, I thought it only considered the most recent keypress.

Gonna have to think about that...

This also makes the non-matching profiles get disabled which I think makes the action more obvious - and gives further instructions on how to reset things... Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

vadi2

Thanks a lot for continuing to investigate this 👍

https://streamable.com/h8y6x2

See one example where I try to find the Materia Magica profile by typing mate repeatedly. It works with Qt, but with our own implementation it keeps finding ateraan instead.

vadi2 · 2021-01-31T06:50:15Z

src/dlgConnectionProfiles.cpp

+    notificationAreaIconLabelError->hide();
+    notificationAreaIconLabelInformation->show();
+    notificationAreaMessageBox->show();
+    notificationAreaMessageBox->setText(tr("As you type more letters in the name of a profile all the non-matching ones "


Let's not show this message, it was not necessary for the last ten years - this is definitely way too much "nagging"

Yeah, but since I am actually disabling non-matching profiles we ought to let the end user know how to get them back again. I suppose I could try something more fancy that would only show this message once per Mudlet run - but I do not really want to add too much more 🍝 code here.

Also - I cannot view streamable.com URLs - they must be (like GitHub) using features that are only compatible with the most bleeding edge browsers and unlike GH (https://github.com/JustOff/github-wc-polyfill) I do not have a fix for them.

Otherwise - is this any good? 😟

The message does not need to be shown at all. Good UX does not require explanation!

SlySven · 2021-01-31T18:38:44Z

See one example where I try to find the Materia Magica profile by typing mate repeatedly. It works with Qt, but with our own implementation it keeps finding ateraan instead.

My best guess is that the m is ending your previous attempt to match something so the a is the first letter of the actual search - if you press <Esc> as suggested to clear any stored keypresses you should find it works as expected.

vadi2 · 2021-01-31T19:24:52Z

Sent a video on Discord that shows the differences, have a look

…rofileIcons Needed to include fix for crashing on exit in TTimerUnit destructor. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

Kebap · 2021-02-01T18:22:57Z

You should maybe put a timer of 2-3 seconds of no keys pressed before starting a new name circle

SlySven · 2021-02-01T19:13:10Z

Actually I did dissect that video on Discord and I eventually worked out that what I suspected was, indeed, the case:

My best guess is that the m is ending your previous attempt to match something so the a is the first letter of the actual search - if you press as suggested to clear any stored keypresses you should find it works as expected.

I really don't want to have to use a timer - as it is I think the visual feed back from all the non-matching profiles becoming disabled (with the message explaining this - but which disappears as soon as the search is cleared by the first letter that does not leave any matching profiles) is a viable scheme as it is...

vadi2 · 2021-02-01T19:27:50Z

The idea was to keep the same behaviour, not change it, because that won't be intuitive and frustrate muscle memory, though.

Kebap · 2021-02-01T19:35:23Z

I did not make up the timer myself: It is instead common best practice in such list selection widgets. Try your file explorer or web browser for good measure. Try the Mudlet map areas selection drop down menu as well. - You can't restart the selection by keep on typing. You need to pause for a moment to have it restart.

SlySven · 2021-02-02T13:56:47Z

You can't restart the selection by keep on typing. You need to pause for a moment to have it restart.

I am not sure I like the idea of using timer dependent code simply because that will need a determination of the time period to use and that is going to be horribly subjective IMVHO - how about if the end user is using assistive technology (or even a screen reader). I feel the enabling/disabling idea is attractive because it is very much more visible - for me the selected item is not that well differentiated under the current release codebase - but with only a few (or ultimately only one) left enabled and not greyed out - it is hard to miss the profile(s) that are possible. I will have a closer look to see exactly how QComboBoxs behave and see if that can be reproduced - as well as the current QListWidget behaviour with text. The latter wasn't as obvious as I believed - I will also need to see how it responded to <BackSpace>/<Del> to see how typos are handled and if possible corrections.

SlySven · 2021-02-02T15:07:58Z

ℹ️ I have just tested the behaviour of the development branch code to see how its QListWidget with text entries behave and I have found that it only uses the last keypress and switches in order through all the profiles that begin with the key pressed! Which is how the original commit db80c46 behaved - not as you suggested @vadi2 that one could type in the whole name and it would select the particular profile.

So how does everyone expect/want this process to work?

vadi2 · 2021-02-02T15:10:32Z

How it's done in Qt, that was the original goal to keep that behaviour. With Qt I can type mate and get the Materia Magica profile - that doesn't seem like what you describe above, so are you sure it works that way?

SlySven · 2021-02-02T20:36:27Z

How it's done in Qt, that was the original goal to keep that behaviour. With Qt I can type mate and get the Materia Magica profile - that doesn't seem like what you describe above, so are you sure it works that way?

That wasn't what I was getting with a build of the development branch - let me do a clean build (and reset my setup to use the system's qtkeychain library so I can still build it with qmake)...

... and yeah - the development code does only consider the last key I press - I cannot spell out the profile name like you say you have it Vadim.

SlySven · 2021-02-02T23:40:40Z

AH! With @Faenriis help I've worked it out - if you type a sequence of keys quickly enough that it does string them together to find a match - however if you do it slower it will reset after a fraction of a second and, as I experienced, jump around the profiles. Maybe @Kebap was right when he talked about a timer that reset things - I was thinking of over a second but in fact it is only a fraction of a second.

Okay with that knowledge I can have another think.

This is to provide the same functionality that the QListWidget does for keypresses, if the keys are typed fast enough they are concatenated to form the text to search for. However if the user types slower than this then the individual keys are only considered on their own. This reflects what both Vadim and I experienced - in that he was typing fast enough to spell out a particular profile but I wasn't and so the profile that I was having selected was changing on each keypress. The time between keypresses allowed so that they can be concatenated I have set to be 1 second. This is significantly slower that the Qt code but it is long enough to permit the temporary disablement of non-matching profiles to be a useful visual feedback - and besides, it makes it easier to specify one of a number of profiles with a common initial text - the longer the timeout the more keys the user can string together! Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

vadi2

I honestly think it's fantastic, and the highlighting makes it even better than the original Qt implementation!

Just the notification should go, it's not all too helpful.

vadi2 · 2021-02-03T17:25:57Z

src/dlgConnectionProfiles.cpp

        mCustomIconColors.append(QColor::fromHsv(i, 128, 255));
    }
+
+    mSearchTextTimer.setInterval(1000);


For easier reading!

Suggested change

mSearchTextTimer.setInterval(1000);

mSearchTextTimer.setInterval(1s);

At the top, add:

#include <chrono> using namespace std::chrono_literals;

I'm getting deja vu all over again about this (std time intervals) - I'm not a fan of them but I guess i can do it... 🙁

vadi2 · 2021-02-03T17:27:01Z

src/dlgConnectionProfiles.cpp

+void dlgConnectionProfiles::slot_searchTimerTimeOut()
+{
+    reenableAllProfileItems();
+    if (notificationAreaMessageBox->text() == tr("As you type more letters in the name of a profile all the non-matching ones "


It's intuitive enough to work with that it needs no explanation - and it's also impossible to read it fast enough :)

Guess so - I must confess that is one incentive to drag the time interval out... 😈 ... but I'll leave the refactoring into clearNotificationArea() as that is still useful I think.

Yes agree, clearNotificationArea() is good!

clearNotificationArea ist super good - do more of those! 🌟

Remove the help text shown during typing the keys to select a profile - the time it is shown for is not enough for it to be that useful. Add `std::chrono` so we can give a time delay as `1s` instead of `1000` mS. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

This finally enables us to do away with the text under the profile icons as it replicates the normal `QListWidget` behaviour to move the selection to each item which begins with the first letter of the key pressed. This also makes the non-matching profiles get disabled which I think makes the action more obvious... Revised to include a timeout on concatenating keypresses: This is to provide the same functionality that the QListWidget does for keypresses, if the keys are typed fast enough they are concatenated to form the text to search for. However if the user types slower than this then the individual keys are only considered on their own. This reflects what both Vadim and I experienced - in that he was typing fast enough to spell out a particular profile but I wasn't and so the profile that I was having selected was changing on each keypress. The time between keypresses allowed so that they can be concatenated I have set to be 1 second. This is significantly slower that the Qt code but it is long enough to permit the temporary disablement of non-matching profiles to be a useful visual feedback - and besides, it makes it easier to specify one of a number of profiles with a common initial text - the longer the timeout the more keys the user can string together! Add `std::chrono` so we can give a time delay as `1s` instead of `1000` mS. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

SlySven added the enhancement label Jan 29, 2021

SlySven added this to the 4.11.0 milestone Jan 29, 2021

SlySven requested a review from a team as a code owner January 29, 2021 11:16

SlySven requested a review from a team January 29, 2021 11:16

SlySven mentioned this pull request Jan 29, 2021

Improve display of profile name below profile picture #4680

Closed

SlySven linked an issue Jan 29, 2021 that may be closed by this pull request

Improve display of profile name below profile picture #4680

Closed

vadi2 reviewed Jan 29, 2021

View reviewed changes

vadi2 assigned SlySven Jan 29, 2021

Revise: take a different approach and enable whole name to be typed

ea38c66

This also makes the non-matching profiles get disabled which I think makes the action more obvious - and gives further instructions on how to reset things... Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

vadi2 reviewed Jan 31, 2021

View reviewed changes

Merge branch 'development' into Enhance_removeMinusculeTextFromUnderP…

0aa0dd1

…rofileIcons Needed to include fix for crashing on exit in TTimerUnit destructor. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

vadi2 approved these changes Feb 3, 2021

View reviewed changes

SlySven merged commit a418d8b into Mudlet:development Feb 3, 2021

SlySven deleted the Enhance_removeMinusculeTextFromUnderProfileIcons branch February 3, 2021 20:00

	mSearchTextTimer.setInterval(1000);
	mSearchTextTimer.setInterval(1s);

Uh oh!

Conversation

SlySven commented Jan 29, 2021

Uh oh!

add-deployment-links bot commented Jan 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test versions

Uh oh!

vadi2 left a comment

Choose a reason for hiding this comment

Uh oh!

SlySven commented Jan 29, 2021

Uh oh!

vadi2 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SlySven commented Jan 31, 2021

Uh oh!

vadi2 commented Jan 31, 2021

Uh oh!

Kebap commented Feb 1, 2021

Uh oh!

SlySven commented Feb 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vadi2 commented Feb 1, 2021

Uh oh!

Kebap commented Feb 1, 2021

Uh oh!

SlySven commented Feb 2, 2021

Uh oh!

SlySven commented Feb 2, 2021

Uh oh!

vadi2 commented Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SlySven commented Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SlySven commented Feb 2, 2021

Uh oh!

vadi2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SlySven Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

add-deployment-links bot commented Jan 29, 2021 •

edited

Loading

vadi2 left a comment •

edited

Loading

SlySven commented Feb 1, 2021 •

edited

Loading

vadi2 commented Feb 2, 2021 •

edited

Loading

SlySven commented Feb 2, 2021 •

edited

Loading

SlySven Feb 3, 2021 •

edited

Loading