Mailinglist Archive: yast-devel (46 mails)

< Previous Next >
Re: [yast-devel] CWM Widgets: a rant ;-)
  • From: Josef Reidinger <jreidinger@xxxxxxx>
  • Date: Tue, 13 Jun 2017 18:25:27 +0200
  • Message-id: <20170613182527.5ece880c@linux-vvcf.privatesite>
On Tue, 13 Jun 2017 16:18:31 +0200
Ancor Gonzalez Sosa <ancor@xxxxxxx> wrote:

I just wanted to share something that has been in my head for some
time. As programmers, we tend to have some personal affection to "our
creatures", so please don't take it too serious. And consider this a
very low-priority mail, compared to other recent mails in this
list. :-)

I have never liked CWM Widgets much and I must say I don't know them
in depth. So I will say many non accurate things in this mail and you
are free to consider this almost as FUD :-). On the other hand, not
being as familiar as the creators can be a good thing - when you get
used to the quirks of a technology you don't see them anymore.

TLDR; Things that really need to be reusable can be made reusable
with a more straightforward and flexible approach. And using a single
"reusable" CWM in your dialog actually enforces all your code
(including the dialog itself) to play by the CWM rules. I'm not sure
if I like that. Although maybe the gain is worth the price and I'm
simply failing to see it.

Now the long version.

I consider the creation of the expert partitioner a great showcase of
the things I dislike from CWM.

There was already a working prototype in yast-storage-ng (and is still
there)[1]. It contains some "debatable" lines of Ruby code and it
rates very low in CodeClimate for many reasons. I didn't write it
and, is you ask me, I don't even like it much. :-) BUT it uses plain
Ruby classes and rely in the nice and straightforward UI::Dialog and
its event loop. You can understand how the code works in 2 minutes.
And if you open a random file, you will know in which part of the
partitioner are you and how the interaction with Y2Storage works
(listing, creating or deleting partitions, LVs, etc.) at first sight.
In one word - straightforward.

Now let's take a look to the CWM-based yast-partitioner[2]. Sure it
does more things and has the limitation of being forced to clone the
not-so-nice UI of the old partitioner. But even having that into
account, the level of complexity there is a little bit daunting. When
reading a piece of code, I never know where it fits in the overall
picture. Martin already suggested to create some kind of "map" for
newcomers with diagrams and links, which is a sign. And all that
because:

1) CWM Widgets are way more than widgets

They take a lot of responsibility and are in full control of the event
loop. They manage user input, route the user between dialogs/widgets,
store the result in the domain classes (or YCP modules), trigger the
real changes in the system... In a MVC pattern (just as a well known
pattern, even if we don't follow it), a single widget will be the
whole V the whole C and a even a significant part of the M.

Hi,
let me try to enlighten some stuff you mention here.
At first I see CWM widgets mostly as controllers. Model should be
passed to it and view is partly driven by qss and partly by CWM like
e.g. order of collumns in table. What is different from e.g. MVC from
rails, is that in CWM it is small controller as it control single
widget, it is not controller for whole dialog. Same applies to input
loop.

In UI:Dialog I solve it by having method per event. So you have
ok_handler if ok is press, but it force responsibility for modification
of widget to dialog, which at least from my POV reduce reusability
there, as you have to reuse whole dialog or copy stuff from it. On
other hand in CWM, widget is responsible for reacting to change.
See e.g.
https://github.com/yast/yast-partitioner/pull/5/files#diff-7687d3be2bf4d672e7e89d1bea5a21e2R28

here button response only for clicking on it. No other events.


2) CWM is a dictator :-)

Maybe as a consequence of the point above, the usage of CWM impose a
concrete way of doing things for your YaST client.

They need extra workarounds to play together with other modern parts
of YaST that embrace OOP. For example, during the development of the
partitioner, somebody wanted to create a dialog just using our beloved
UI:Dialog class. To make it possible we ended up creating a CWM widget
that represents a dialog, so the dialog is contained in the widgets
system (and not the other way around) and then it can play by the CWM
rules and be used.

It is caused by fact that both UI::Dialog and CWM need to control UI
loop. I also agree that we can make CWM easier to use outside of CWM.
Basically only missing part is to generate UI::Term for content and
recognize if event is for me. Otherwise init is own method, store is own
method, validate is also there, help is there, so you just need to call
it.

Basically idea is this ( adding TODO for missing functionality )

content = HBox(
...
widget.content, # TODO
...
)
UI.OpenDialog(content)
widget.init
UI.loop do |e|
res = widget.handle # TODO throw away other event
return res if res
end
if success
loop_again unless widget.validate
widget.store
end

and all that handle, validate and store only if widget need it, of
course.

So if there is an interest, it can be extended to be used also this way.


I also have the impression that the CWM model encourages too much the
usage of a Singleton approach in the "models layer". Something that
made a lot of sense for YCP modules (that are singleton by
definition), but I don't like that much with OOP. But this can, of
course, be just a subjective impression.

Actually original idea was completely different. Pass model to
CWMWidget and also in partitioner original idea was to use
Y2Storage::Disk, Y2Storage::Partition, etc. as models, until we
recognize that without device graph which cannot be accessed from
model, we cannot act on it.

see e.g.
https://github.com/yast/yast-partitioner/blob/master/src/lib/y2partitioner/widgets/disk_bar_graph.rb
it do not access anything else then disk passed there.


3) Reusability can be easier than that

We already have examples of reusable "widgets" that don't embrace the
CWM model in YaST. Like the widget to stop/start/enable/disable
services, the recent presenter to show a list of actions in the
devicegraph with the ability to collapse/extend some parts or even
some parts of the mentioned expert partitioner prototype.

just note, we are talking about
https://github.com/yast/yast-yast2/blob/master/library/general/src/lib/ui/service_status.rb

So what problems I see there?
It force reload flag, even when I do not want it. It is also 206 LOC,
which is not so small, compare it e.g. to
https://github.com/yast/yast-installation/blob/master/src/lib/installation/widgets/online_repos.rb
which is only single button, but it manage everything and you have it
specify only core ( label and what to do after click ).

And the main problem? there is already methods for various widgets
( all ending with *_widget ), but cannot be used independently if I
want to reuse only part of it. In fact this class looks almost like
CWM, just with more glue around.

For mentioned prototype I do not see how to reuse single widget, only I
see how to reuse whole dialog.


They are usually quite simple and straightforward. A initializer to
set the initial state or to pass the object(s) they "wrap", a handler
for the user input, a method returning the current tree of UI
terms... and that's usually it. They are compatible with any possible
approach used in the UI, not imposing a given paradigm. They can be
used inside a CWM-style dialog, inside a UI:Dialog, directly in a
proposal client, etc. because the don't over-do.

See above, same can be done with CWM if you want to use it in common
dialog ( not so hard to modify it for usage ), but on top of it, allows
easier constructing of more complex widgets or dialogs with

content = Hbox(
widget1,
widget2,
widget3
)




4) Relatively complex mechanism even for simple stuff

I have mixing feelings about this point. On the one hand, I kind of
like the CWM approach of using classes for everything. On the other
hand, sometimes it feels overkill for many cases to define a new
class and then instantiate a new object for very simple tasks like
adding a button. That's specially true for stuff that you don't
really want to be reusable, but that you want to fit in the whole
picture of classes and subclasses. In general, adding a button or a
label is usually not as easy as... well, adding a button or a
label. :-)

Is adding button really so easy? button is not just its label, but also
have to react on clicking on it, which is usually non-trivial. so for
button it is in old way:

content = HBox (
...
PushButton(Id(:delete), _("Delete Partition")),
...
)

UI.loop do |e|
if e == :delete
... # several lines
else
...
end

how it looks in CWM? this:
https://github.com/yast/yast-partitioner/pull/5/files#diff-7687d3be2bf4d672e7e89d1bea5a21e2R9

label is simple one liner, but rest of functionality is handle, which
reacts on pressing button.

It include several features like getting device from table, getting it
directly, showing fancy popup and others. And all of this we can reuse
on multiple place. And almost without dependencies. So trivial ones
like label is already trivial and usually rest are more tricky.
You can check trivial widgets in bootloader, which is just about
trivial input field or number widget and try to find any line that is
not needed for its functionality ( and not because CWM require it, but
because it is real widget specific functionality ).

https://github.com/yast/yast-bootloader/blob/master/src/lib/bootloader/grub2_widgets.rb#L42



Pheew! Feel much better now. ;) Deactivating rant mode.

Cheers.

[1]
https://github.com/yast/yast-storage-ng/tree/master/src/lib/expert_partitioner
[2] https://github.com/yast/yast-partitioner/ (many things are still
not merged in master at the time of writing)

--
To unsubscribe, e-mail: yast-devel+unsubscribe@xxxxxxxxxxxx
To contact the owner, e-mail: yast-devel+owner@xxxxxxxxxxxx

< Previous Next >
References