New Partitioner UI based on dynamic menu-buttons #752

ancorgs · 2018-10-01T08:58:01Z

Main PR for https://trello.com/c/5rrmzwKh/276-3-partitioner-unleash-the-beast

This implements the UI described in this gist, check that document for the rationale.

The new UI fits into an 80x24 text console and allows all kind of operations. For example, it's possible to start with three empty disks and end up creating this complex setup using only the Partitioner.

There you can see:

/dev/md0 is an MD RAID defined on top of two partitions and formatted as "/" (nothing really new here),
/dev/md1 is an MD RAID defined on top of a combination of full disks (/dev/sdb) and partitions (/dev/sda3) and containing partitions like /dev/md1p1 and /dev/md1p2 itself (two things that were not possible in the old Partitioner),
/dev/volgroup0 is an LVM VG based on one of those MD partitions,
/dev/sdc is a disk hosting directly a file-system with no partitions in between (also a new possibility).

And this is how some parts of the process to create such setup look like in a text console.

Directly formatting a disk (no partitions):

Playing with partitions.

Creating an MD RAID on top of two full disks and then creating partitions within the RAID.

Last but not least, that's how those screens look in graphical mode (SLE-15-SP1 installation).

jreidinger · 2018-10-01T09:11:42Z

src/lib/y2partitioner/widgets/device_buttons_set.rb

-          partition_buttons
-        else
-          raid_buttons
+        types = [:partition, :software_raid, :lvm_vg, :lvm_lv, :stray_blk_device]


this looks like constant for list of supported device types?

jreidinger · 2018-10-01T10:10:14Z

src/lib/y2partitioner/widgets/device_menu_button.rb

+        working_graph.find_device(device_sid)
+      end
+
+      # @return [Array<[Symbol, String]>] list of menu options


well, this comment is a bit imprecise. I prefer here simple see for CWM method which defines how it exactly have to look.

jreidinger · 2018-10-01T10:11:39Z

src/lib/y2partitioner/widgets/device_menu_button.rb

+      #
+      # @return [Boolean]
+      def validate_presence
+        return true unless device.nil?


NP: but me is more nature return true if device

jreidinger · 2018-10-01T10:53:04Z

src/lib/y2partitioner/widgets/lvm_devices_table.rb

@@ -34,7 +34,8 @@ class LvmDevicesTable < ConfigurableBlkDevicesTable
      # @param devices [Array<Y2Storage::Lvm_vg, Y2Storage::Lvm_lv>] devices to display
      # @param pager [CWM::Pager] table have feature, that double click change content of pager
      #   if someone do not need this feature, make it only optional
-      def initialize(devices, pager)
+      # @param buttons_set [DeviceButtonsSet]


please document also why it is needed.

Actually, that whole documentation should not be there, in LvmDevicesTable, which is not the base class. #devices and #pager are already (better) documented in the base class.

jreidinger · 2018-10-01T11:10:03Z

src/lib/y2partitioner/widgets/partition_add_button.rb

-      def initialize(pager: nil, table: nil, device: nil, short: false)
-        super(pager: pager, table: table, device: device)
-        @short_label = short
+      # Constructor. See parent class.


I worry this see won;t create automatic link in yardoc

jreidinger · 2018-10-01T11:29:36Z

src/lib/y2partitioner/dialogs/md.rb

@@ -33,6 +33,8 @@
 #   @see http://www.rubydoc.info/github/yast/yast-yast2/CWM%2FCustomWidget:${0}
 # @!macro [new] seeDialog
 #   @see http://www.rubydoc.info/github/yast/yast-yast2/CWM%2FDialog:${0}
+# @!macro [new] seeItemsSelection
+#   @see http://www.rubydoc.info/github/yast/yast-yast2/CWM%2FItemsSelection:${0}


it looks familiar for me :) https://github.com/yast/yast-storage-ng/pull/746/files#diff-e9f6c27fd34d282cdefbf6f09306cccdR36

jreidinger · 2018-10-02T12:21:29Z

src/lib/y2partitioner/widgets/pages/disks.rb

+
+        # @see DevicesTable
+        def table_buttons
+          Empty()


hmm, what about making this default behavior in base and redefine only when needed?

jreidinger · 2018-10-02T12:23:23Z

src/lib/y2partitioner/ui_state.rb

+    #
+    # @return [String]
+    def self.md_raids_label
+      N_("RAID")


why it is not constant? We usually use N_ in constants. and _ in common methods. This usage of N_ and _ in caller looks strange for me.

I just moved stuff from the previous place in a hurry (kind of). With UIState being a singleton class, it makes indeed no sense to keep the old N_ thingie.

jreidinger · 2018-10-02T12:24:02Z

src/lib/y2partitioner/widgets/pages/lvm.rb

        # @macro seeAbstractWidget
        def label
-          _(self.class.label)
+          _(UIState.lvm_label)


if label already have _() instead of N_ then we do not need to translate it again here.

jreidinger · 2018-10-02T14:02:57Z

src/lib/y2partitioner/widgets/device_menu_button.rb

@@ -129,6 +129,7 @@ def widget_id_for_action(action)
      # @param id [Symbol, String]
      # @return [Hash]
      def action_for_widget_id(id)
+        return nil if id.nil?


should be nil or exception? if so, then please document it that it can also return nil.

Actually, I would never expect #handle to be called with no arguments (or with a nil argument) since handle_all_events is used.

Unfortunately, supporting that case is needed in order to be able to use include_examples "CWM::AbstractWidget" which always calls #handle with no arguments, so I have to adapt the class to ignore such wrong calls.

ah, yeah. That is problematic, because for calls it uses arity and rspec mock basically mocks call with *args, so -1 arity and it breaks this test. Please add comment for it, as it is quite tricky.

jreidinger · 2018-10-02T14:03:17Z

package/yast2-storage-ng.spec

@@ -16,7 +16,7 @@
 #

 Name:		yast2-storage-ng
-Version:	4.1.20
+Version:        4.1.21


we should finally fix that indentation issue :)

yast-bot · 2018-10-02T14:26:46Z

✔️ Internal Jenkins job #46 successfully finished
✔️ Created IBS submit request #173325

yast-bot · 2018-10-02T14:27:34Z

✔️ Public Jenkins job #42 successfully finished
✔️ Created OBS submit request #639614

Sometime ago[1][2], the .label method from some pages was moved to the UIState in order to avoid a dependency cycles. Since that is not the case anymore, this commit "restore" the .label method for those pages. [1] #752 [2] b107fc5

Sometime ago[1][2], the .label method from some pages was moved to the UIState in order to avoid a dependency cycles. However, it was a kind of temporary fix and it's time to really solve the problem. As first step, this commit "restore" the .label method for those pages. Subsequent commit(s) will avoid mentioned dependency. [1] #752 [2] b107fc5

jreidinger reviewed Oct 1, 2018

View reviewed changes

ancorgs force-pushed the menu-buttons branch from 9f5f9a0 to e7e5d7e Compare October 1, 2018 09:12

jreidinger reviewed Oct 1, 2018

View reviewed changes

ancorgs force-pushed the menu-buttons branch 2 times, most recently from 14e5416 to 2053c58 Compare October 1, 2018 13:33

ancorgs added 5 commits October 1, 2018 15:46

The Edit button always edits

8f28560

Allow to select the next tab by its label

930c7c4

Remove :short option from PartitionAddButton

3d245ca

Actions executed from the table select the proper row at the end

9effec6

Do not open disks in the Partitions tab by default

c661458

ancorgs force-pushed the menu-buttons branch from 2053c58 to 2fdb15d Compare October 1, 2018 13:52

yast deleted a comment from coveralls Oct 1, 2018

ancorgs added 14 commits October 2, 2018 14:00

Use DeviceButtonsSet in all tables except system

950c892

Use DeviceResizeButton only for resizing, not to change PVs

ec5bf85

Fix wrong memoization

f1094b8

Redefine buttons in the different pages

e318e56

Remove not longer used buttons

bc25efc

Simplify DeviceButton, searching in table is not used

60aa9a4

Handle StrayBlkDevice in the Hard Disks table

5ae257b

Adapt Partitioner unit tests to the new UI

efe7f98

Some code improvements

ebe0b34

Add some missing unit test

1949922

Do not re-document what is documented in base classes

7e8e997

Adapt bcache table to the new UI

47674c8

Pages::DevicesTable as base class for 4 pages

a4ba9e6

Avoid cyclic dependency of UIState and Pages

b107fc5

ancorgs force-pushed the menu-buttons branch from 60cb0bc to b107fc5 Compare October 2, 2018 12:01

jreidinger reviewed Oct 2, 2018

View reviewed changes

ancorgs added 3 commits October 2, 2018 15:11

Code improvements

e01da15

Delete All button in LVs list (consistent with partitions list)

90fa0b9

More robust and tested DeviceMenuButton

59a7b2c

jreidinger reviewed Oct 2, 2018

View reviewed changes

ancorgs added 2 commits October 2, 2018 16:10

Version and changelog

af5f9c1

Improve method documentation

9c39c04

ancorgs force-pushed the menu-buttons branch from 20b50c8 to 9c39c04 Compare October 2, 2018 14:11

jreidinger approved these changes Oct 2, 2018

View reviewed changes

ancorgs merged commit 99c4a57 into yast:master Oct 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Partitioner UI based on dynamic menu-buttons #752

New Partitioner UI based on dynamic menu-buttons #752

ancorgs commented Oct 1, 2018 •

edited

Loading

jreidinger Oct 1, 2018

jreidinger Oct 1, 2018

jreidinger Oct 1, 2018

jreidinger Oct 1, 2018

ancorgs Oct 1, 2018

jreidinger Oct 1, 2018

jreidinger Oct 1, 2018

jreidinger Oct 2, 2018

jreidinger Oct 2, 2018

ancorgs Oct 2, 2018

jreidinger Oct 2, 2018

jreidinger Oct 2, 2018

ancorgs Oct 2, 2018

jreidinger Oct 2, 2018

jreidinger Oct 2, 2018

yast-bot commented Oct 2, 2018

yast-bot commented Oct 2, 2018

New Partitioner UI based on dynamic menu-buttons #752

New Partitioner UI based on dynamic menu-buttons #752

Conversation

ancorgs commented Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yast-bot commented Oct 2, 2018

yast-bot commented Oct 2, 2018

ancorgs commented Oct 1, 2018 •

edited

Loading