Merge 5f268fd into 82939e0

package/yast2-installation.changes

@@ -1,4 +1,11 @@
 -------------------------------------------------------------------
+Sun Nov 10 18:56:53 UTC 2019 - David Diaz <dgonzalez@suse.com>
+
+- Improve the role selection dialog using a new scrollable widget
+  to select a role (related to bsc#1084674, bsc#bsc#1086187).
+- 4.2.21
+-------------------------------------------------------------------
+
 Thu Nov  7 09:32:00 UTC 2019 - Ladislav Slezák <lslezak@suse.cz>
 
 - Implement upgrade for the Full medium (jsc#SLE-7101)

package/yast2-installation.spec

@@ -16,7 +16,7 @@
 #
 
 Name:           yast2-installation
-Version:        4.2.20
+Version:        4.2.21
 Release:        0
 Group:          System/YaST
 License:        GPL-2.0-only
@@ -94,6 +94,8 @@ Requires:       iproute2
 %endif
 
 
+# SingleItemSelector not enforcing an initial selection
+Conflicts:      libyui < 3.8.2
 # Pkg::SourceProvideSignedFile Pkg::SourceProvideDigestedFile
 # pkg-bindings are not directly required
 Conflicts:      yast2-pkg-bindings < 2.17.25

src/lib/installation/select_system_role.rb

@@ -17,12 +17,10 @@
 #  To contact SUSE about this file by physical or electronic mail,
 #  you may find current contact information at www.suse.com
 
-require "cgi"
 require "yast"
 require "ui/installation_dialog"
 require "installation/services"
 require "installation/system_role"
-require "ui/text_helpers"
 
 Yast.import "GetInstArgs"
 Yast.import "Packages"
@@ -33,8 +31,6 @@
 
 module Installation
   class SelectSystemRole < ::UI::InstallationDialog
-    include UI::TextHelpers
-
     NON_OVERLAY_ATTRIBUTES = [
       "additional_dialogs",
       "id",
@@ -81,21 +77,28 @@ def help_text
     end
 
     def dialog_content
-      @selected_role_id = SystemRole.current
-      @selected_role_id ||= roles.first && roles.first.id if SystemRole.default?
+      preselected_role_id = SystemRole.current
+      preselected_role_id ||= roles.first && roles.first.id if SystemRole.default?
 
-      HCenter(ReplacePoint(Id(:rp), role_buttons(selected_role_id: @selected_role_id)))
+      VBox(
+        Left(Label(Yast::ProductControl.GetTranslatedText("roles_text"))),
+        VSpacing(2),
+        SingleItemSelector(
+          Id(:role_selector),
+          roles_items(preselected_role_id)
+        )
+      )
     end
 
     def create_dialog
       clear_role
       ok = super
-      Yast::UI.SetFocus(Id(:roles_richtext)) if ok
+      Yast::UI.SetFocus(Id(:role_selector)) if ok
       ok
     end
 
     def next_handler
-      result = select_role(@selected_role_id)
+      result = select_role(selected_role_id)
       # We show the main role dialog; but the additional clients have
       # drawn over it, so draw it again and go back to input loop.
       # create_dialog do not create new dialog if it already exist like in this
@@ -108,20 +111,30 @@ def next_handler
       end
     end
 
-    # called if a specific FOO_handler is not defined
-    def handle_event(id)
-      role = SystemRole.find(id)
-      if role.nil?
-        log.info "Not a role: #{id.inspect}, skipping"
-        return
-      end
+  private
 
-      @selected_role_id = id
-      Yast::UI.ReplaceWidget(Id(:rp), role_buttons(selected_role_id: id))
-      Yast::UI.SetFocus(Id(:roles_richtext))
+    # Return a collection holding items to build the role selector
+    #
+    # @param preselected_role_id [String, nil] the id of the role that should be selected
+    # @return [Array<Item>] collection of role items
+    def roles_items(preselected_role_id)
+      roles.map do |role|
+        Item(
+          Id(role.id),
+          role.label,
+          # Keep the description's line lenght short enough to avoid horizontal scroll in 80x24
+          role.description.strip.scan(/.{1,70}\W/).join("\n"),
+          role.id == preselected_role_id
+        )
+      end
     end
 
-  private
+    # Return the curren selected role id
+    #
+    # @return [String]
+    def selected_role_id
+      Yast::UI.QueryWidget(Id(:role_selector), :Value)
+    end
 
     # checks if there is only one role available
     def single_role?
@@ -263,68 +276,5 @@ def roles(refresh: false)
       SystemRole.clear if refresh
       SystemRole.all
     end
-
-    # Returns the content for the role buttons
-    # @param selected_role_id [String] which role radiobutton gets selected
-    # @return [Yast::Term] Role buttons
-    def role_buttons(selected_role_id:)
-      role_rt_radios = roles.map do |role|
-        # FIXME: following workaround can be removed as soon as bsc#997402 is fixed:
-        # bsc#995082: System role descriptions use a character that is missing in console font
-        description = Yast::UI.TextMode ? role.description.tr("•", "*") : role.description
-
-        rb = richtext_radiobutton(id:       role.id,
-                                  label:    role.label,
-                                  selected: role.id == selected_role_id)
-
-        description = CGI.escape_html(description).gsub("\n", "<br>\n")
-        # extra empty paragraphs for better spacing
-        "<p></p>#{rb}<p></p><ul>#{description}</ul>"
-      end
-
-      intro_text = Yast::ProductControl.GetTranslatedText("roles_text")
-      VBox(
-        Left(Label(intro_text)),
-        VSpacing(2),
-        RichText(Id(:roles_richtext), div_with_direction(role_rt_radios.join("\n")))
-      )
-    end
-
-    def richtext_radiobutton(id:, label:, selected:)
-      if Yast::UI.TextMode
-        richtext_radiobutton_tui(id: id, label: label, selected: selected)
-      else
-        richtext_radiobutton_gui(id: id, label: label, selected: selected)
-      end
-    end
-
-    def richtext_radiobutton_tui(id:, label:, selected:)
-      check = selected ? "(x)" : "( )"
-      widget = "#{check} #{CGI.escape_html(label)}"
-      enabled_widget = "<a href=\"#{id}\">#{widget}</a>"
-      "#{enabled_widget}<br>"
-    end
-
-    IMAGE_DIR = "/usr/share/YaST2/theme/current/wizard".freeze
-
-    BUTTON_ON = "◉".freeze # U+25C9 Fisheye
-    BUTTON_OFF = "○".freeze # U+25CB White Circle
-
-    def richtext_radiobutton_gui(id:, label:, selected:)
-      # check for installation style, which is dark, FIXME: find better way
-      installation = ENV["Y2STYLE"] == "installation.qss"
-      if installation
-        image = selected ? "inst_radio-button-checked.png" : "inst_radio-button-unchecked.png"
-        # NOTE: due to a Qt bug, the first image does not get rendered properly. So we are
-        # rendering it twice (one with height and width set to "0").
-        bullet = "<img src=\"#{IMAGE_DIR}/#{image}\" height=\"0\" width=\"0\"></img>" \
-                 "<img src=\"#{IMAGE_DIR}/#{image}\"></img>"
-      else
-        bullet = selected ? BUTTON_ON : BUTTON_OFF
-      end
-      widget = "#{bullet} #{CGI.escape_html(label)}"
-      enabled_widget = "<a class='dontlooklikealink' href=\"#{id}\">#{widget}</a>"
-      "<p>#{enabled_widget}</p>"
-    end
   end
 end

test/select_system_role_test.rb

@@ -134,69 +134,82 @@
             "software" => { "desktop" => "knome" }, "additional_dialogs" => "a,b" }
         ]
       end
+      let(:user_input) { :next }
+      let(:going_back) { false }
+      let(:selected_role_id) { nil }
+      let(:current_role_id) { nil }
 
       before do
-        allow(Yast::ProductControl).to receive(:system_roles)
-          .and_return(control_file_roles)
+        allow(Yast::GetInstArgs).to receive(:going_back).and_return(going_back)
+
+        allow(Yast::ProductControl).to receive(:system_roles).and_return(control_file_roles)
+
+        allow(Installation::SystemRole).to receive(:current).and_return(current_role_id)
+
         allow(Installation::SystemRole).to receive(:select).with("foo")
           .and_return(Installation::SystemRole.new(id: "foo", order: 100))
         allow(Installation::SystemRole).to receive(:select).with("bar")
           .and_return(Installation::SystemRole.new(id: "bar", order: 200))
-      end
 
-      it "displays dialog, and sets ProductFeatures on Next" do
         allow(Yast::Wizard).to receive(:SetContents)
-        allow(Yast::UI).to receive(:UserInput)
-          .and_return("foo", :next)
+        allow(Yast::UI).to receive(:UserInput).and_return(*user_input)
 
-        expect(Yast::ProductFeatures).to receive(:ClearOverlay)
-        expect(Yast::ProductFeatures).to receive(:SetOverlay)
-
-        expect(subject.run).to eq(:next)
+        allow(subject).to receive(:selected_role_id).and_return(selected_role_id)
       end
 
-      it "displays dialog, and leaves ProductFeatures on Back" do
-        allow(Yast::Wizard).to receive(:SetContents)
-        allow(Yast::UI).to receive(:UserInput)
-          .and_return(:back)
-        expect(Yast::ProductFeatures).to receive(:ClearOverlay)
-        expect(Yast::ProductFeatures).to_not receive(:SetOverlay)
+      context "when user goes forward" do
+        let(:selected_role_id) { "foo" }
 
-        expect(subject.run).to eq(:back)
+        it "displays dialog and sets ProductFeatures" do
+          expect(Yast::ProductFeatures).to receive(:ClearOverlay)
+          expect(Yast::ProductFeatures).to receive(:SetOverlay)
+
+          expect(subject.run).to eq(:next)
+        end
+
+        context "and the role contains additional dialogs" do
+          let(:selected_role_id) { "bar" }
+
+          it "shows the first dialog" do
+            allow(Yast::WFM).to receive(:CallFunction).and_return(:next)
+            expect(Yast::WFM).to receive(:CallFunction).with("a", anything).and_return(:next)
+
+            expect(subject.run).to eq(:next)
+          end
+        end
       end
 
-      context "when a role contains additional dialogs" do
-        it "shows the first dialog when going forward" do
-          allow(Yast::Wizard).to receive(:SetContents)
-          allow(Yast::UI).to receive(:UserInput)
-            .and_return("bar", :next)
+      context "when user goes back" do
+        let(:current_role_id) { "bar" }
+        let(:user_input) { :back }
 
-          allow(Yast::WFM).to receive(:CallFunction).and_return(:next)
-          expect(Yast::WFM).to receive(:CallFunction).with("a", anything).and_return(:next)
+        it "displays dialog and leaves ProductFeatures" do
+          expect(Yast::ProductFeatures).to receive(:ClearOverlay)
+          expect(Yast::ProductFeatures).to_not receive(:SetOverlay)
 
-          expect(subject.run).to eq(:next)
+          expect(subject.run).to eq(:back)
         end
 
-        it "shows the last dialog when going back" do
-          allow(Installation::SystemRole).to receive(:current).and_return("bar")
-          allow(Yast::GetInstArgs).to receive(:going_back).and_return(true)
-          expect(Yast::Wizard).to_not receive(:SetContents)
-          expect(Yast::UI).to_not receive(:UserInput)
-          expect(Yast::UI).to_not receive(:QueryWidget)
+        context "and the role contains additional dialogs" do
+          let(:going_back) { true }
 
-          expect(Yast::WFM).to receive(:CallFunction).with("b", anything).and_return(:next)
+          it "shows the last dialog" do
+            expect(Yast::Wizard).to_not receive(:SetContents)
+            expect(Yast::UI).to_not receive(:UserInput)
+            expect(Yast::UI).to_not receive(:QueryWidget)
 
-          expect(subject.run).to eq(:next)
+            expect(Yast::WFM).to receive(:CallFunction).with("b", anything).and_return(:next)
+
+            expect(subject.run).to eq(:next)
+          end
         end
       end
 
       context "when re-selecting the same role" do
-        it "just proceeds without a popup" do
-          allow(Installation::SystemRole).to receive(:current).and_return("foo")
-          allow(Yast::Wizard).to receive(:SetContents)
-          allow(Yast::UI).to receive(:UserInput)
-            .and_return("foo", :next)
+        let(:current_role_id) { "foo" }
+        let(:selected_role_id) { "foo" }
 
+        it "just proceeds without a popup" do
           expect(Yast::Popup).to_not receive(:ContinueCancel)
 
           expect(Yast::ProductFeatures).to receive(:ClearOverlay)
@@ -207,12 +220,11 @@
       end
 
       context "when re-selecting a different role" do
-        it "displays a popup, and proceeds if Continue is answered" do
-          allow(Installation::SystemRole).to receive(:current).and_return("bar")
-          allow(Yast::Wizard).to receive(:SetContents)
-          allow(Yast::UI).to receive(:UserInput)
-            .and_return("foo", :next)
+        let(:current_role_id) { "bar" }
+        let(:selected_role_id) { "foo" }
+        let(:user_input) { [:next, :back] }
 
+        it "displays a popup, and proceeds if Continue is answered" do
           expect(Yast::Popup).to receive(:ContinueCancel)
             .and_return(true)
 
@@ -223,11 +235,6 @@
         end
 
         it "displays a popup, and does not proceed if Cancel is answered" do
-          allow(Installation::SystemRole).to receive(:current).and_return("bar")
-          allow(Yast::Wizard).to receive(:SetContents)
-          allow(Yast::UI).to receive(:UserInput)
-            .and_return("foo", :next, :back)
-
           expect(Yast::Popup).to receive(:ContinueCancel)
             .and_return(false)
           expect(Yast::ProductFeatures).to receive(:ClearOverlay)
@@ -238,15 +245,10 @@
       end
 
       context "when no roles is selected" do
-        it "shows error and does not continue" do
-          allow(Yast::Wizard).to receive(:SetContents)
-          allow(Yast::UI).to receive(:UserInput)
-            .and_return(:next, :back)
-          allow(Yast::UI).to receive(:QueryWidget)
-            .with(Id(:roles), :CurrentButton).and_return(nil)
-          allow(Installation::SystemRole).to receive(:default?)
-            .and_return(false)
+        let(:selected_role_id) { nil }
+        let(:user_input) { [:next, :back] }
 
+        it "shows error and does not continue" do
           expect(Yast::Popup).to receive(:Error)
           expect(Yast::ProductFeatures).to receive(:ClearOverlay)
           expect(Yast::ProductFeatures).to_not receive(:SetOverlay)