CWM::Dialog; shared_examples for more widgets #592

mvidner · 2017-06-19T08:49:16Z

These are needed for the Expert Partitioner, and were introduced in yast/yast-partitioner#5 (because we had not branched SP3 back then)

CWM::Dialog
UI::Sequence
RSpec.shared_examples for more CWM widgets
UI::Greasemonkey

coveralls · 2017-06-19T08:55:41Z

Coverage decreased (-0.09%) to 27.659% when pulling a2dfe53 on cwm-rspec2 into 15350bb on master.

with RSpec.shared_examples too

coveralls · 2017-06-19T14:16:03Z

Coverage increased (+0.2%) to 27.994% when pulling ba55f90 on cwm-rspec2 into 15350bb on master.

which is in turn adapted from GreasemonkeyClass in yast2-storage

coveralls · 2017-06-22T11:09:03Z

Coverage increased (+0.4%) to 28.19% when pulling 09bb403 on cwm-rspec2 into 15350bb on master.

jreidinger · 2017-06-22T11:22:52Z

library/general/src/lib/ui/greasemonkey.rb

+module UI
+  # UI layout helpers.
+  #
+  module Greasemonkey


if you want to add GreaseMonkey to yast2, then significant improvement of code quality is needed ;) as yast2 mean that it will be used on more places and there is highed requirements. Also documentation have to be significantly improved and same for test coverage.

As for test coverage, I can only offer 100% ;-)

As for the API: I need it to stay compatible, because that is the point: reusing pieces of the old dialogs while I concentrate on other things. Actually, in yast/yast-partitioner#5 I used this but then in yast/yast-partitioner#15 nothing is used as I gradually rewrote the dialogs. I expect several recurrences of this.

I will add docs.

well, when we add it to yast2, it is like we are suggesting to reuse it. So I think for this case functional API make sense as it is conversion of one kind of term to other, but I really like to have code that does not look like ycp :)

jreidinger · 2017-06-22T11:26:23Z

library/general/test/greasemonkey_test.rb

+
+describe "UI::Greasemonkey" do
+  RSpec.shared_examples "a Greasemonkey method" do |mname|
+    it "transforms its argument properly" do


it should better document what happen like it "transforms GreaseMonkey term to UI term"

also what about adding describe ".#{mname}" do around it?

OK

No, this is done by the describes below.

still missing 1. fixed. It should mention that output of greasemonkey method should be valid UI term.

jreidinger · 2017-06-22T11:29:59Z

library/cwm/src/lib/cwm/dialog.rb

+    include Yast::UIShortcuts
+
+    # @return [String,nil] Set a title, or keep the existing title
+    abstract_method :title


does it make sense to have method that return nil? like def title; nil; end? why not having nil as default?

jreidinger · 2017-06-22T11:31:41Z

library/cwm/src/lib/cwm/dialog.rb

+      Yast::Wizard.CloseDialog
+    end
+
+    def run_assuming_open


I really do not like this name. Also why it is public method? and why it missing documentation if it is intention to be a public method?

I suggest something like def show

jreidinger · 2017-06-22T11:31:56Z

library/cwm/src/lib/cwm/dialog.rb

+      end
+    end
+
+    def wizard_create_dialog(&block)


missing documentation, also why it is public method?

jreidinger · 2017-06-22T11:33:18Z

library/cwm/src/lib/cwm/dialog.rb

+      true
+    end
+
+    # @return [Array<Symbol>]


at least link to cwm documentation for it

and in fact, it doesn't have to be onlu symbol, it is in fact Array<any non nil object>

OK.

I think you are wrong about this:

yast-yast2/library/cwm/src/modules/CWM.rb

Line 877 in 15350bb

Convert.to_symbol(ret)

enforces a Symbol.

well, in fact I do not test it. But from past I know that any object stop during handle and convert to symbol looks happen only after test for skip for store

yast-yast2/library/cwm/src/modules/CWM.rb

Line 874 in 15350bb

saveWidgets(widgets, event_descr) if save && !skip_store_for.include?(ret)

Parse error.
Anyway, a non-symbol means store may run, so the dialog "works", but nil is returned from show and I think such usage is wrong and should be fixed.

on tricky value is string, which is converted, so if handle return "test" skip for have to have "test", but return value is :test. But I agree that we should not support it.

jreidinger · 2017-06-22T11:34:54Z

library/general/src/lib/ui/greasemonkey.rb

+
+# File:	Greasemonkey.ycp
+# Package:	yast2-storage
+# Summary:	Expert Partitioner


these three lines are no longer valid.

jreidinger · 2017-06-22T11:35:03Z

library/general/src/lib/ui/greasemonkey.rb

+# Summary:	Expert Partitioner
+# Authors:	Arvin Schnell <aschnell@suse.de>
+#
+# Lets see if this turns out to be useful.


jreidinger · 2017-06-22T11:35:22Z

library/general/src/lib/ui/greasemonkey.rb

+require "yast"
+
+module UI
+  # UI layout helpers.


this need really more extensive documentation

jreidinger · 2017-06-22T11:38:50Z

library/sequencer/src/lib/ui/sequence.rb

+require "yast"
+Yast.import "Sequencer"
+
+# FIXME: once the API is reviewed, move this to yast-yast2


no longer valid

jreidinger · 2017-06-22T11:39:34Z

library/sequencer/src/lib/ui/sequence.rb

+    # {Yast::SequencerClass#Run Yast::Sequencer.Run}
+    # but smarter:
+    # - auto :abort (see {#abortable})
+    # - *aliases* are assumed to be method names if unpecified


typo unpecified

jreidinger · 2017-06-22T11:40:05Z

library/sequencer/src/lib/ui/sequence.rb

+    # - *aliases* are assumed to be method names if unpecified
+    #   (see {#from_methods})
+    def run(aliases: nil, sequence:)
+      aliases = from_methods(sequence) if aliases.nil?


can be written as aliases ||= from_methods(sequence)

jreidinger · 2017-06-22T11:41:08Z

library/sequencer/src/lib/ui/sequence.rb

+    # (an :abort from a dialog should :abort the whole sequence)
+    def abortable(sequence_hash)
+      sequence_hash.map do |name, destination|
+        if name == "ws_start"


this "hack" should be documented why it is needed. Also it smells for me like constant.

It is not a hack, it is a standard part of the sequence hash that specifies the starting point.

Unfortunately the docs for Sequencer itself is ... bad.

so use constant with proper name like "INITIAL_DIALOG"?

jreidinger · 2017-06-22T11:43:31Z

library/sequencer/src/lib/ui/sequence.rb

+      # (also see Yast::SequencerClass#WS_special)
+      def skip_stack(method_symbol)
+        @skip_stack ||= {}
+        @skip_stack[method_symbol] = true


to be honest I do not get why it is class method and not instance one as I expect that skipped methods are single run specific, not? so what happen if two or three Sequence is used in row?

You have a point. IIUC you don't want FooSequence to affect BarSequence. I believe this will not happen, because I have used a class-level @attribute, not a @@class-attribute (which is shared across the hierarchy). I will add a test for it.

It is a class method because I need it at the class level. Actual usage:

class AddPartition < UI::Sequence def preconditions if slots.empty? Yast::Popup.Error("...") return :back end :next end skip_stack :preconditions def type Dialogs::PartitionType.run(disk, @ptemplate, @slots) end def size Dialogs::PartitionSize.run(disk, @ptemplate, @slots) end ...

so it is class attribute in fact. Well, it make sense then, if it is subclass specific attribute, that is persistent accross same instances.

btw you can write it as skip_stack def preconditions

jreidinger · 2017-06-22T11:43:57Z

library/sequencer/src/lib/ui/sequence.rb

+        @skip_stack[method_symbol] = true
+      end
+
+      def skip_stack?(name)


documentation

test isolation of skip_stack

coveralls · 2017-06-23T07:40:32Z

Coverage increased (+0.6%) to 28.411% when pulling a0bfee4 on cwm-rspec2 into ed43046 on master.

this should fix Travis

coveralls · 2017-06-23T11:42:19Z

Coverage increased (+0.6%) to 28.411% when pulling 878a7dc on cwm-rspec2 into ed43046 on master.

coveralls · 2017-06-23T12:37:25Z

Coverage increased (+0.6%) to 28.393% when pulling 912b950 on cwm-rspec2 into ed43046 on master.

jreidinger · 2017-06-23T15:28:33Z

library/general/src/lib/ui/greasemonkey.rb

+    # @example
+    #   term(:FrameWithMarginBox, "Title", "arg1", "arg2")
+    #      ->
+    #   Frame("Title", MarginBox(1.45, 0.45, "arg1", "arg2"))


seen this usage, I think it make sense to mention it as old example and deprecated. I think prefered way to use should be

include GreaseMonkey def contents VBox( FrameWithMarginBox("Title", "arg1", "arg2") ) end

and this should be a priority example for suggested usage. Or how do you see it?

That is out of scope for this PR. As I say in the module header,

These started out in the Expert Partitioner in yast2-storage.
The use case is reusing pieces of this legacy code in the new yast2-partitioner.
That is why the API and the implementation look old.

if only use case is reusing code in yast2-partitioner, that why it is in yast2 where it should serve all modules? That is why I am more strict regarding API and other stuff, because one module scope it is ok, if there are some legacy stuff, but if we add it to yast2 itself, it means we have to be more careful in future when we want to improve it.

jreidinger · 2017-06-23T15:34:54Z

library/sequencer/test/sequence_test.rb

+        "first" => [subj.method(:first), true]
+      }
+
+      expect(subj.from_methods(seq)).to eq(wanted)


I do not get this test. I expect it should test if TestSequence do not skip "first", not? Here it test that class that explicitly skip it really skip it, which is not what "it" description mention

it does fail if @skip_stack is replaced with @@skip_stack

coveralls · 2017-06-26T09:18:24Z

Coverage increased (+0.6%) to 28.393% when pulling cd83bfe on cwm-rspec2 into ed43046 on master.

jreidinger · 2017-06-26T14:02:28Z

library/cwm/src/lib/cwm/dialog.rb

+
+    # @return [Array<Symbol>]
+    #   Events for which `store` won't be called, see {Yast::CWMClass#show}
+    def skip_store_for


question is for which actions it should be done. Currently only use case I see is :redraw. So maybe this should be redraw_event and also handle it in run? Just idea as @teclator currently solving something similar.

jreidinger · 2017-06-26T14:08:43Z

library/sequencer/src/lib/ui/sequence.rb

+  # A {UI::Sequence} is an object-oriented interface for the good old
+  # {Yast::SequencerClass Yast::Sequencer}.
+  # In the simple case it runs a sequence of dialogs
+  # connected by Back and Next buttons.


as sequencer can be a bit tricky, I really welcome here one or two trivial examples of usage

jreidinger

two documenting NPs and I am still not convinced that it is good idea to add to YaST2 itself greasemonkey beast without API and code improvements, but in the end LGTM

yast/yast-yast2#592

RSpec.shared_examples for CWM: Page, PushButton, RadioButtons, RichText.

48eba86

mvidner mentioned this pull request Jun 19, 2017

Use yast2.rpm for CWM::Dialog yast/yast-partitioner#16

Merged

mvidner added 4 commits June 19, 2017 16:05

Added CWM::Dialog

c56eb0b

with RSpec.shared_examples too

Better docs for CWM::Dialog

5ffc2bf

version + changelog

962388b

Added UI::Sequence, an OOP API for Yast::Sequencer

ba55f90

mvidner force-pushed the cwm-rspec2 branch from a2dfe53 to ba55f90 Compare June 19, 2017 14:06

mvidner added 2 commits June 22, 2017 10:35

Copied UI::Greasemonkey from yast2-partitioner

6aaca54

which is in turn adapted from GreasemonkeyClass in yast2-storage

Tests for UI::Greasemonkey.

09bb403

jreidinger reviewed Jun 22, 2017

View reviewed changes

library/sequencer/src/lib/ui/sequence.rb Outdated

@skip_stack[method_symbol] = true

end

def skip_stack?(name)

Copy link

Member

jreidinger Jun 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

documentation

mvidner added 3 commits June 22, 2017 14:39

CWM::Dialog: #title=nil, fewer public methods, tests.

7b22790

UI::Sequence: better docs, START = "ws_start"

01fcdaf

test isolation of skip_stack

UI::Greasemonkey docs

0fc5093

mvidner added 3 commits June 23, 2017 09:27

Merge branch 'master' into cwm-rspec2

1fbdb1f

Mention UI::Sequence, UI::Greasemonkey

b3ccd0f

Whitespace cleanup

a0bfee4

mvidner added 2 commits June 23, 2017 13:35

Fixup yardoc links

f2e29ca

Use the standard test helper

878a7dc

this should fix Travis

mvidner added 2 commits June 23, 2017 14:30

UI::Greasemonkey: more docs

d0c3508

UI::Greasemonkey: do not include the entire Yast namespace

912b950

mvidner force-pushed the cwm-rspec2 branch from 2c550f7 to 912b950 Compare June 23, 2017 12:30

jreidinger reviewed Jun 23, 2017

View reviewed changes

Clearer test for not confusing skip_stack

cd83bfe

it does fail if @skip_stack is replaced with @@skip_stack

jreidinger reviewed Jun 26, 2017

View reviewed changes

jreidinger approved these changes Jun 26, 2017

View reviewed changes

mvidner merged commit f13ddf9 into master Jun 27, 2017

mvidner deleted the cwm-rspec2 branch June 27, 2017 06:41

mvidner mentioned this pull request Jun 29, 2017

Added a bug number #594

Merged

mvidner added a commit to yast/yast-partitioner that referenced this pull request Jun 29, 2017

Moved UI::Sequence to yast2.rpm

5284cb3

yast/yast-yast2#592

CWM::Dialog; shared_examples for more widgets #592

CWM::Dialog; shared_examples for more widgets #592

Conversation

mvidner commented Jun 19, 2017 • edited Loading

coveralls commented Jun 19, 2017

coveralls commented Jun 19, 2017

coveralls commented Jun 22, 2017

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

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

coveralls commented Jun 23, 2017

coveralls commented Jun 23, 2017

coveralls commented Jun 23, 2017

jreidinger Jun 23, 2017 • 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

coveralls commented Jun 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreidinger left a comment

Choose a reason for hiding this comment

mvidner commented Jun 19, 2017 •

edited

Loading

jreidinger Jun 23, 2017 •

edited

Loading