Skip to content
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

Improve the role selection dialog #827

Merged
merged 7 commits into from Nov 14, 2019
Merged

Improve the role selection dialog #827

merged 7 commits into from Nov 14, 2019

Conversation

@dgdavid
Copy link
Member

dgdavid commented Nov 7, 2019

Context

Some time ago, the list of available roles grown enough to start not fitting in the role selection dialog. To fix it, the list was embedded in a scrollable RichText widget since libyui didn't offer a better option (see #671, bsc#1084674, bsc#1086187).

Trello card: https://trello.com/c/QjRVileZ

Current status

As part of a YaST UI improvements a new SingleItemSelector (see libyui/libyui#148) has been added, which is a much better solution to display the roles list.

Changes

To update the role selection dialog using the new widget and removing the old code.

Tests

  • Unit tests updated/adapted
  • Also tested manually via driver update.

Screenshots

Click to show/hide

Before

ncurses Qt
Current role selection dialog ncurse Current role selection dialog
ncurses 80x25 ncurses 80x25 scrolling
ncurses 80x25 ncurses 80x25 scrolling

After

ncurses Qt
Selection role dialog using the new widget ncurses Selection role dialog using the new widget
ncurses 80x25 ncurses 80x25 scrolling
ncurses 80x25 ncurses 80x25 scrolling
Qt 1920x1080 Qt 2560x1600
Qt 1920x1080 Qt 2560x1600
@dgdavid dgdavid force-pushed the change-role-selector branch from 18e7af8 to bc5c062 Nov 7, 2019
@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 7, 2019

Coverage Status

Coverage decreased (-0.1%) to 23.921% when pulling dca6922 on change-role-selector into 82939e0 on master.

@dgdavid dgdavid force-pushed the change-role-selector branch 3 times, most recently from 7238dbb to edafd7e Nov 8, 2019
@dgdavid dgdavid changed the title [WIP] Change role selector Improve the role selector Nov 11, 2019
@dgdavid dgdavid changed the title Improve the role selector Improve the role selection dialog Nov 11, 2019
@dgdavid dgdavid force-pushed the change-role-selector branch from 9a73a59 to d78f1e9 Nov 11, 2019
@dgdavid dgdavid marked this pull request as ready for review Nov 11, 2019
@dgdavid dgdavid force-pushed the change-role-selector branch from d78f1e9 to 5f268fd Nov 11, 2019
@dgdavid dgdavid requested review from lslezak, imobachgs and shundhammer Nov 11, 2019
@shundhammer

This comment has been minimized.

Copy link
Contributor

shundhammer commented Nov 11, 2019

Technically, LGTM. Just the one typo in the comment.
We might think about the forced line break; it makes perfect sense in NCurses, but I am not so sure about Qt. But that's more the icing on the cake than a deal breaker.
I'd also like to have @lslezak look at this; he might have more input.

@dgdavid

This comment has been minimized.

Copy link
Member Author

dgdavid commented Nov 11, 2019

Thanks so much for the review!

Technically, LGTM. Just the one typo in the comment.
We might think about the forced line break; it makes perfect sense in NCurses, but I am not so sure about Qt.

The thing is that some role description also force the horizontal scroll in Qt. I'll take an screenshot as soon as possible.

But that's more the icing on the cake than a deal breaker.
I'd also like to have @lslezak look at this; he might have more input.

Perfect!

dgdavid added 4 commits Nov 7, 2019
Because the SingleItemSelector widget (fixed to not enforce an initial
selection) is now used to build the role selection dialog.
@dgdavid dgdavid force-pushed the change-role-selector branch from 5f268fd to b91b2f6 Nov 12, 2019
@dgdavid

This comment has been minimized.

Copy link
Member Author

dgdavid commented Nov 12, 2019

I'll take an screenshot as soon as possible.

See the comment above

@dgdavid dgdavid force-pushed the change-role-selector branch 3 times, most recently from 17537ec to a35dd2a Nov 13, 2019
dgdavid added 2 commits Nov 13, 2019
As suggested in the code review, now the max line length is longer when
not running in text mode.
Likely forgotten in f1b4e40
@dgdavid dgdavid force-pushed the change-role-selector branch from a35dd2a to 663a11a Nov 13, 2019
def adjust_text(text)
max_line_length = Yast::UI.TextMode ? TEXT_MODE_MAX_LINE_LENGTH : MAX_LINE_LENGTH

text.strip.scan(/(.{1,#{max_line_length}})(?:\s|$)/).join("\n")

This comment has been minimized.

Copy link
@dgdavid

dgdavid Nov 13, 2019

Author Member

JFYI, I also tried

text.strip.gsub(/(.{1,#{max_line_length}})(?:\s|$)/, "\\1\n").chomp

but a benchmark reveal that scan and join allocates less memory than gsub and chomp. Exactly 1.11x less, although it will almost priceless for this use case...

This comment has been minimized.

Copy link
@lslezak

lslezak Nov 14, 2019

Member

I just noticed that this does not work well if a word is longer than the max. line length:

max_line_length = 10
text = "123456789012 123456789012 123456789012 123456789012"
puts text.strip.scan(/(.{1,#{max_line_length}})(?:\s|$)/).join("\n")

results in

3456789012
3456789012
3456789012
3456789012

i.e. some part is lost. But as using a word longer than at least 70 characters is not very likely we can live with that. But maybe it's worth adding a comment about this...

This comment has been minimized.

Copy link
@dgdavid

dgdavid Nov 14, 2019

Author Member

hint, you can use existing code https://github.com/yast/yast-yast2/blob/66bc64d08c6ff017b88e3cff63fd02d78bbcf7f1/library/general/src/lib/ui/text_helpers.rb#L31

Ops! I didn't know about it. And... I didn't took a look around 😓

Thanks!

@dgdavid

This comment has been minimized.

Copy link
Member Author

dgdavid commented Nov 13, 2019

@shundhammer, @lslezak

I re-tested it with the new changes (basically different max line lengths for textmode) and updated the most relevant screenshots in the PR description.

So, It's ready for a re-review, please.

@shundhammer

This comment has been minimized.

Copy link
Contributor

shundhammer commented Nov 13, 2019

Still LGTM from my side. @lslezak ?

Copy link
Member

lslezak left a comment

LGTM.

But as this is mainly about the UI it would be nice to see some screenshots here...

def adjust_text(text)
max_line_length = Yast::UI.TextMode ? TEXT_MODE_MAX_LINE_LENGTH : MAX_LINE_LENGTH

text.strip.scan(/(.{1,#{max_line_length}})(?:\s|$)/).join("\n")

This comment has been minimized.

Copy link
@lslezak

lslezak Nov 14, 2019

Member

I just noticed that this does not work well if a word is longer than the max. line length:

max_line_length = 10
text = "123456789012 123456789012 123456789012 123456789012"
puts text.strip.scan(/(.{1,#{max_line_length}})(?:\s|$)/).join("\n")

results in

3456789012
3456789012
3456789012
3456789012

i.e. some part is lost. But as using a word longer than at least 70 characters is not very likely we can live with that. But maybe it's worth adding a comment about this...

@lslezak

This comment has been minimized.

Copy link
Member

lslezak commented Nov 14, 2019

Oops, the screenshots are already there... 🤦‍♂

@dgdavid

This comment has been minimized.

Copy link
Member Author

dgdavid commented Nov 14, 2019

I just noticed that this does not work well if a word is longer than the max. line length:

Yep, I was trying to solve this problem in different ways, but as you said

using a word longer than at least 70 characters is not very likely

reason why I decided to not invest much time on it. And, for the same reason, I don't want to solve this with a loop...

@dgdavid

This comment has been minimized.

Copy link
Member Author

dgdavid commented Nov 14, 2019

Yep, I was trying to solve this problem in different ways, but as you said

using a word longer than at least 70 characters is not very likely

reason why I decided to not invest much time on it. And, for the same reason, I don't want to solve this with a loop...

That said, I didn't know (my fault, for not asking or taken a look) about the helper pointed out by Josef (see his comment)

So, I'm going to update the code to use it.

@dgdavid

This comment has been minimized.

Copy link
Member Author

dgdavid commented Nov 14, 2019

😫 😫 wrap_text helper don't respect the carriage returns inside the text 😞

using wrap_text

@dgdavid

This comment has been minimized.

Copy link
Member Author

dgdavid commented Nov 14, 2019

😫 😫 wrap_text helper don't respect the carriage returns inside the text 😞

Anyway, as we agreed in IRC, I'm goint to merge this, then continue with #825 and finally improve the helper to support this use case.

@dgdavid dgdavid merged commit 61db415 into master Nov 14, 2019
4 checks passed
4 checks passed
Docker Build Task Summary
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 23.921%
Details
@dgdavid dgdavid deleted the change-role-selector branch Nov 14, 2019
@yast-bot

This comment has been minimized.

Copy link

yast-bot commented Nov 14, 2019

✔️ Public Jenkins job #76 successfully finished
✔️ Created OBS submit request #748578

@yast-bot

This comment has been minimized.

Copy link

yast-bot commented Nov 14, 2019

✔️ Internal Jenkins job #17 successfully finished
✔️ Created IBS submit request #205113

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.