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

Allow to select the license language when running in text mode #441

Merged
merged 23 commits into from
Jun 17, 2019

Conversation

dgdavid
Copy link
Member

@dgdavid dgdavid commented May 22, 2019

Product License Hard to Understand? Try Another Language!

Problem: Only in the text-mode installation, we cannot switch the language of a product license, both for the main product and add-on products. The language switching widget is there but it's disabled.bsc#1135901

eula-language

Reason: That's on purpose, since on the Linux console we're not able to display the characters of most languages, bsc#1094793.

Fix: But wait, recently we have switched to starting fbiterm unconditionally, so we are in fact able to display all languages that currently have license translations. So let's enable the language switch.

@dgdavid dgdavid requested a review from imobachgs May 22, 2019 14:27
@mvidner
Copy link
Member

mvidner commented May 22, 2019

See also another check for TextMode:

if Yast::UI.TextMode

def force_language?
return false unless Yast::UI.TextMode

Yast::Builtins.getenv("TERM") == "iterm"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other way around!


# Whether it is needed to force the default language
#
# @return [Boolean] true if running but not in a fbiterm; false otherwise
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if running what

@dgdavid
Copy link
Member Author

dgdavid commented May 22, 2019

@mvidner, @lslezak Now the PR is ready for review. Thanks!

@dgdavid dgdavid marked this pull request as ready for review May 22, 2019 15:33
@mvidner
Copy link
Member

mvidner commented May 22, 2019

There are more checks for TextMode in ProductLicense:

if Yast::UI.TextMode

and

license_locale = (Yast::UI.TextMode && locale.empty?) ? default_language : locale

@coveralls
Copy link

coveralls commented May 22, 2019

Coverage Status

Coverage increased (+0.05%) to 29.585% when pulling 56e20da on feature/fix-bsc-1135901 into 6fa0d56 on SLE-15-SP1.

@mvidner
Copy link
Member

mvidner commented May 23, 2019

My WIP is c067f5d It fails the old unit tests, and changes only the main product, but its ideas are clear enough to be transferable to the add-on product case in ProductLicense.rb

@mvidner
Copy link
Member

mvidner commented May 23, 2019

TODO: things to test:

  • license available
  • language displayable
  • lang_code_match("zh", "zh_CN"): is it symmetric? "pt_BR" vs "pt-br"
  • "" is English (WTF)

@kobliha
Copy link
Member

kobliha commented May 24, 2019

@mvidner "" is "en_US" as that one has been chosen as the default language.

@mvidner mvidner changed the title Do not allow to select a different language when not running on fbiterm Allow to select the license language when running in text mode Jun 14, 2019
RSpec::Matchers.define :array_not_including do |x|
match do |actual|
return false unless actual.is_a?(Array)
return false if actual.include?(x)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a simple !actual.include?(x) instead of the 2 lines?

Copy link
Member

@lslezak lslezak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just some minor notes/questions.

# 2 space indentation
[*.rb]
indent_style = space
indent_size = 2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about adding also trim_trailing_whitespace = true?

  • I'm not sure if we want to add it as a part of a maintenance update, maybe adding only to master would be better.
  • It would make sense to have it in all repositories, but then we should agree on the exact content first (discuss at yast-devel@ ?)

Copy link
Member

@mvidner mvidner Jun 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll bring it up on the mailing list... done.

src/lib/language_tag.rb Outdated Show resolved Hide resolved
test/lib/widgets/product_license_translations_test.rb Outdated Show resolved Hide resolved
test/lib/widgets/product_license_translations_test.rb Outdated Show resolved Hide resolved
Co-Authored-By: Ladislav Slezák <lslezak@suse.cz>
# @return [LanguageTag,nil]
def generalize
self.class.new(@tag.split("_").first) if @tag.include? "_"
# else nil
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seem to be dead code. We could just remove it :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Um, I'd actually put some comment that nil might be returned above the code line. If you just quickly scan the line you might miss that if at the end and think that an object is always returned.

test/lib/widgets/product_license_translations_test.rb Outdated Show resolved Hide resolved
@mvidner mvidner merged commit f71e94a into SLE-15-SP1 Jun 17, 2019
@mvidner mvidner deleted the feature/fix-bsc-1135901 branch June 17, 2019 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants