Merge 3121a69 into cea4b45

package/yast2-storage-ng.changes

@@ -1,3 +1,13 @@
+-------------------------------------------------------------------
+Mon Apr 22 22:56:55 UTC 2019 - David Diaz <dgonzalez@suse.com>
+
+- Partitioner: by default, initialize the tree with sections
+  expanded and devices collapsed.
+- Partitioner: improves the user experience preserving, between
+  every redraw, the tree items as the were: expanded or collapsed.
+- Lightly related to fate#318196.
+- 4.2.7
+
 -------------------------------------------------------------------
 Mon Apr 15 15:46:04 UTC 2019 - ancor@suse.com
 

package/yast2-storage-ng.spec

@@ -16,7 +16,7 @@
 #
 
 Name:		yast2-storage-ng
-Version:	4.2.6
+Version:	4.2.7
 Release:	0
 
 BuildRoot:	%{_tmppath}/%{name}-%{version}-build

src/lib/y2partitioner/dialogs/main.rb

@@ -57,10 +57,17 @@ def title
       end
 
       def contents
+        # NOTE: Since this method is used as first parameter of {Yast::CWM.show} every time that
+        # {#run} calls `super`, a new {OverviewTreePager} will be created in every dialog redraw.
+        # So, let's keep a reference to it in the {UIState} to query the open items and preserve
+        # their state in new instances.
+        overview_tree_pager = Widgets::OverviewTreePager.new(hostname)
+        UIState.instance.overview_tree_pager = overview_tree_pager
+
         MarginBox(
           0.5,
           0.5,
-          Widgets::OverviewTreePager.new(hostname)
+          overview_tree_pager
         )
       end
 

src/lib/y2partitioner/ui_state.rb

@@ -32,9 +32,25 @@ class UIState
     # means default for each widget will be honored).
     def initialize
       textdomain "storage"
+
       @candidate_nodes = []
+      @open_items_ids = nil
+      @overview_tree_pager = nil
     end
 
+    # A reference to the overview tree pager, which is a new instance every dialog redraw. See note
+    # in {Dialogs::Main#contents}
+    #
+    # @return [Widgets::OverviewTreePager]
+    attr_accessor :overview_tree_pager
+
+    # Items of the tree that should be expanded in the next redraw.
+    # @note Nil means that open items are not being saved yet and the default tree state should be
+    # used instead. See {Widgets::OverviewTreePager#item_open?}
+    #
+    # @return [nil, Array<String,Symbol>]
+    attr_reader :open_items_ids
+
     # Title of the section listing the MD RAIDs
     #
     # @note This is defined in this class as the simplest way to avoid
@@ -133,6 +149,13 @@ def find_tab(pages)
       pages.find { |page| page.label == tab }
     end
 
+    # Stores the ids of the tree items that are open
+    def save_open_items
+      return unless overview_tree_pager
+
+      @open_items_ids = overview_tree_pager.open_items_ids
+    end
+
   protected
 
     # Where to place the user within the general tree in next redraw

src/lib/y2partitioner/widgets/device_button.rb

@@ -42,6 +42,9 @@ def initialize(pager: nil, device: nil)
       # @macro seeAbstractWidget
       def handle
         return nil unless validate_presence
+
+        UIState.instance.save_open_items
+
         actions
       end
 

src/lib/y2partitioner/widgets/device_menu_button.rb

@@ -48,6 +48,8 @@ def initialize(device)
       def handle(event)
         return nil unless validate_presence
 
+        UIState.instance.save_open_items
+
         action = action_for_widget_id(event["ID"])
         return nil unless action
 

src/lib/y2partitioner/widgets/overview.rb

@@ -86,8 +86,6 @@ def items
           *graph_items,
           summary_item,
           settings_item
-          # TODO: Bring this back to life - disabled for now (bsc#1078849)
-          # item_for("settings", _("Settings"), icon: Icons::SETTINGS)
         ]
       end
 
@@ -104,6 +102,15 @@ def initial_page
         UIState.instance.find_tree_node(@pages) || super
       end
 
+      # Obtains the ids of open/expanded items
+      #
+      # @return [Array<String,Symbol>] ids of open/expanded items
+      def open_items_ids
+        Yast::UI
+          .QueryWidget(Id(tree.widget_id), :OpenItems)
+          .keys
+      end
+
       # Obtains the page associated to a specific device
       # @return [CWM::Page, nil]
       def device_page(device)
@@ -125,6 +132,8 @@ def validate
 
     private
 
+      attr_reader :tree
+
       # Checks whether the current setup is valid, that is, it contains necessary
       # devices for booting (e.g., /boot/efi) and for the system runs properly (e.g., /).
       #
@@ -177,114 +186,115 @@ def device_graph
       def system_items
         page = Pages::System.new(hostname, self)
         children = [
-          disks_items,
-          raids_items,
-          lvm_items,
-          bcache_items,
+          disks_section,
+          raids_section,
+          lvm_section,
+          bcache_section,
           # TODO: Bring this back to life - disabled for now (bsc#1078849)
           # crypt_files_items,
           # device_mapper_items,
-          nfs_items,
-          btrfs_items
+          nfs_section,
+          btrfs_section
           # TODO: Bring this back to life - disabled for now (bsc#1078849)
           # unused_items
         ].compact
-        CWM::PagerTreeItem.new(page, children: children, icon: Icons::ALL)
+
+        section_item(page, Icons::ALL, children: children)
       end
 
       # @return [CWM::PagerTreeItem]
-      def disks_items
+      def disks_section
         devices = device_graph.disk_devices + device_graph.stray_blk_devices
 
         page = Pages::Disks.new(devices, self)
         children = devices.map do |dev|
           dev.is?(:stray_blk_device) ? stray_blk_device_item(dev) : disk_items(dev)
         end
-        CWM::PagerTreeItem.new(page, children: children, icon: Icons::HD)
+        section_item(page, Icons::HD, children: children)
       end
 
       # @return [CWM::PagerTreeItem]
       def disk_items(disk, page_class = Pages::Disk)
         page = page_class.new(disk, self)
         children = disk.partitions.sort_by(&:number).map { |p| partition_items(p) }
-        CWM::PagerTreeItem.new(page, children: children)
+        device_item(page, children: children)
       end
 
       # @return [CWM::PagerTreeItem]
       def partition_items(partition)
         page = Pages::Partition.new(partition)
-        CWM::PagerTreeItem.new(page)
+        device_item(page)
       end
 
       # @return [CWM::PagerTreeItem]
       def stray_blk_device_item(device)
         page = Pages::StrayBlkDevice.new(device)
-        CWM::PagerTreeItem.new(page)
+        device_item(page)
       end
 
       # @return [CWM::PagerTreeItem]
-      def raids_items
+      def raids_section
         devices = device_graph.software_raids
         page = Pages::MdRaids.new(self)
         children = devices.map { |m| raid_items(m) }
-        CWM::PagerTreeItem.new(page, children: children, icon: Icons::RAID)
+        section_item(page, Icons::RAID, children: children)
       end
 
       # @return [CWM::PagerTreeItem]
       def raid_items(md)
         page = Pages::MdRaid.new(md, self)
         children = md.partitions.sort_by(&:number).map { |p| partition_items(p) }
-        CWM::PagerTreeItem.new(page, children: children)
+        device_item(page, children: children)
       end
 
       # @return [CWM::PagerTreeItem]
-      def bcache_items
+      def bcache_section
         return nil unless Y2Storage::Bcache.supported?
         devices = device_graph.bcaches
         page = Pages::Bcaches.new(devices, self)
         children = devices.map { |v| disk_items(v, Pages::Bcache) }
-        CWM::PagerTreeItem.new(page, children: children, icon: Icons::BCACHE)
+        section_item(page, Icons::BCACHE, children: children)
       end
 
       # @return [CWM::PagerTreeItem]
-      def lvm_items
+      def lvm_section
         devices = device_graph.lvm_vgs
         page = Pages::Lvm.new(self)
         children = devices.map { |v| lvm_vg_items(v) }
-        CWM::PagerTreeItem.new(page, children: children, icon: Icons::LVM)
+        section_item(page, Icons::LVM, children: children)
       end
 
       # @return [CWM::PagerTreeItem]
       def lvm_vg_items(vg)
         page = Pages::LvmVg.new(vg, self)
         children = vg.all_lvm_lvs.sort_by(&:lv_name).map { |l| lvm_lv_items(l) }
-        CWM::PagerTreeItem.new(page, children: children)
+        device_item(page, children: children)
       end
 
       # @return [CWM::PagerTreeItem]
       def lvm_lv_items(lv)
         page = Pages::LvmLv.new(lv)
-        CWM::PagerTreeItem.new(page)
+        device_item(page)
       end
 
       # @return [CWM::PagerTreeItem]
-      def nfs_items
+      def nfs_section
         page = Pages::NfsMounts.new(self)
-        CWM::PagerTreeItem.new(page, icon: Icons::NFS)
+        section_item(page, Icons::NFS)
       end
 
       # @return [CWM::PagerTreeItem]
-      def btrfs_items
+      def btrfs_section
         page = Pages::Btrfs.new(self)
-        CWM::PagerTreeItem.new(page, icon: Icons::BTRFS)
+        section_item(page, Icons::BTRFS)
       end
 
       # @return [Array<CWM::PagerTreeItem>]
       def graph_items
         return [] unless Yast::UI.HasSpecialWidget(:Graph)
 
         page = Pages::DeviceGraph.new(self)
-        dev_item = CWM::PagerTreeItem.new(page, icon: Icons::GRAPH)
+        dev_item = section_item(page, Icons::GRAPH)
         # TODO: Bring this back to life - disabled for now (bsc#1078849)
         # mount_item = item_for("mountgraph", _("Mount Graph"), icon: Icons::GRAPH)
         # [dev_item, mount_item]
@@ -293,12 +303,56 @@ def graph_items
 
       # @return [CWM::PagerTreeItem]
       def summary_item
-        CWM::PagerTreeItem.new(Pages::Summary.new, icon: Icons::SUMMARY)
+        page = Pages::Summary.new
+        section_item(page, Icons::SUMMARY)
       end
 
       # @return [CWM::PagerTreeItem]
       def settings_item
-        CWM::PagerTreeItem.new(Pages::Settings.new, icon: Icons::SETTINGS)
+        page = Pages::Settings.new
+        section_item(page, Icons::SETTINGS)
+      end
+
+      # Generates a `section` tree item for given page
+      #
+      # The OverviewTreePager has two kinds of items: section or device. Sections always has icon
+      # and starts expanded; devices has not icon and starts collapsed. See also {device_item}.
+      #
+      # @param page [CWM::Page]
+      # @param icon [Icons]
+      # @param children [Array<CWM::PagerTreeItem>]
+      #
+      # @return [CWM::PagerTreeItem]
+      def section_item(page, icon, children: [])
+        CWM::PagerTreeItem.new(page, children: children, icon: icon, open: item_open?(page, true))
+      end
+
+      # Generates a `device` tree item for given page
+      #
+      # @see #section_item
+      #
+      # @param page [CWM::Page]
+      # @param children [Array<CWM::PagerTreeItem>]
+      #
+      # @return [CWM::PagerTreeItem]
+      def device_item(page, children: [])
+        CWM::PagerTreeItem.new(page, children: children, open: item_open?(page, false))
+      end
+
+      # Whether the tree item for given page should be open (expanded) or closed (collapsed)
+      #
+      # When open items are not initialized, the default value will be used. For better
+      # understanding, see the note about the {OverviewTreePager} redrawing in
+      # {Dialogs::Main#contents}
+      #
+      # @param page [CWM::Page]
+      # @param default [Boolean] value when open items are not initialized yet
+      #
+      # @return [Boolean] true when item must be expanded; false if must be collapsed
+      def item_open?(page, default)
+        return default if UIState.instance.open_items_ids.nil?
+
+        UIState.instance.open_items_ids.include?(page.widget_id)
       end
     end
   end

test/y2partitioner/ui_state_test.rb

@@ -345,4 +345,34 @@
       end
     end
   end
+
+  describe "#open_items_ids" do
+    context "if the open items has not been saved" do
+      before { described_class.create_instance }
+
+      it "returns nil" do
+        expect(ui_state.open_items_ids).to be_nil
+      end
+    end
+
+    context "after calling #save_open_items" do
+      let(:pager) { double("TreePager") }
+
+      before do
+        # The first call returns [:first, :third] and the second returns [:second]
+        allow(pager).to receive(:open_items_ids)
+          .and_return([:first, :third], [:second])
+
+        ui_state.overview_tree_pager = pager
+        ui_state.save_open_items
+      end
+
+      it "returns the items that were expanded when #save_open_items was called" do
+        expect(ui_state.open_items_ids).to eq [:first, :third]
+      end
+
+      # To ensure this does not interfere with other tests
+      after { ui_state.overview_tree_pager = nil }
+    end
+  end
 end