On Tue, 13 Jun 2017 16:18:31 +0200
Ancor Gonzalez Sosa
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-7687d3be2bf4d672e... 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/w... 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/se... 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/w... 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-7687d3be2bf4d672e... 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...
Pheew! Feel much better now. ;) Deactivating rant mode.
Cheers.
[1] https://github.com/yast/yast-storage-ng/tree/master/src/lib/expert_partition... [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@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org