Re: [lmi] [PATCH] wx_test_calculation_summary.cpp

[Greg Chicares]

On 2015-03-08 01:09, Vadim Zeitlin wrote:
>  Hello again,
> 
>  I'm attaching the patch implementing the revised specification for this
> test case.

Committed 20150308T2306Z, revision 6126.

> AFAICS it does follow it faithfully, but there are 2 remaining
> questions/potential problems: first one is the previously mentioned issue
> with getting the default summary columns and is discussed in more details
> below.
> 
>  The second one is similar to the previously discussed issues due to
> external files modification in the default update test. Namely, with this
> test, the "distribution only" part of it can pass at most once because it
> relies on the fixed values of the "calculation_summary_columns" and
> "use_builtin_calculation_summary" keys in the configuration file which are
> changed by this test itself. So after running successfully once, the test
> is _guaranteed_ to fail when run the next time with the "--distribution"
> option. As previously stated, this might not be a problem in your workflow,
> but I wanted to at least mention this because it does seem rather
> user/tester-unfriendly to me.

I'll have to confer with Kim.

> On Sat, 07 Mar 2015 00:38:03 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> >  I have a question about default_calculation_summary_columns(): this is 
> a
> GC> > private function in configurable_settings.cpp which is inaccessible to 
> the
> GC> > testing code. Does its mention here mean that I have the licence to 
> make it
> GC> > public and use it? Or should I try, as usual, to make the test work 
> without
> GC> > any changes to the main program code?
> GC> 
> GC> Can we do it with friendship?
> 
>  Yes, we can, but, to be honest, I don't like it very much: IMO the program
> code shouldn't have any knowledge of/coupling with the test code and even
> if a friend declaration is one of the weakest forms of coupling there is,
> it still bothers me.

I'm surprised that you don't like it. Unit tests may need access to
internals, so it seems (to me) perfectly natural for a component to
extend friendship to its own unit tests.

>  IMHO just making default_calculation_summary_columns() (and it only)
> public would be better: as long as some other class (the friend) can access
> it, it's not really fully encapsulated any more anyhow, e.g. you would have
> to update the code using it outside of the class if it ever changes. And in
> this case why not just allow anybody to call it, it's a pure function
> calling which can hardly do any harm.
> 
> GC> > (b) Use effective_calculation_summary_columns() to access the default
> GC> >     columns via a "backdoor"
> ...
> GC> I think it's pretty robust: I can't see any reason for ever changing
> GC> its behavior.
> GC> 
> GC> So my preference order is:
> GC>   best: friendship
> GC>   next best: (b) [sneak through existing back door]
> GC>   second worst: (a) [dubious and fragile code duplication]
> GC>   worst: break encapsulation with s/private:/public:/ [FORTRAN, not OOP]
> 
>  So, taking into account the reassurance above, my preferred solution is
> now (b) and so this is what the patch attached to this message implements.
> If it seems relatively acceptable to you, I'd like to keep it like this but
> if not, I can change the code to use friendship, of course, it would be
> trivial to do.

I'm not going to object to that: it makes the production system no worse.

I might object to the existing backdoor, but I'm probably its author, it
has been there for a long time, and I'm not going to spend time thinking
about removing it now.

> GC> > GC> /// File | Preferences
> GC> > GC> /// uncheck "Use built-in calculation summary"
> GC> > GC> /// set all "Column" controls to "[none]"
> GC> > GC> ///   do this by simulating {Tab Home} repeatedly because that is
> GC> > GC> ///   much faster and is what a smart user does
> GC> > 
> GC> >  In principle, there is no guarantee that doing this would work on all
> GC> > platforms, e.g. it doesn't work under OS X by default.
> GC> 
> GC> I'm just curious, because OS X is often cited as a paragon of usability:
> 
>  Not of _keyboard_ usability though, this is somehow not something that is
> deemed to be important for the normal users,

Wow.

> apparently just knowing about
> the Ctrl-C/Ctrl-V key combinations

[ugly msw replacements for the intuitive SAA CUA Ctrl-Ins and Shift-Ins]

> is a mark of a power user nowadays. IMO
> OS X is the [mainstream] OS most unfriendly to the keyboard users. Sadly,
> even Linux/GTK+ has many problems in this area and IME MSW remains the best
> UI from this point of view.

Well, there's OS/2. Or there was.

[snip fascinating further discussion of OS X keyboard-unfriendliness]

> GC> Instead of these two prescriptive lines:
> GC> > GC> ///   do this by simulating {Tab Home} repeatedly because that is
> GC> > GC> ///   much faster and is what a smart user does
> GC> let me present my thoughts descriptively.
> GC> 
> GC> Watching this test run, I formed an impression that the code was stepping
> GC> through each combobox item, as if it did this:
> GC>     loop0: select previous or next item
> GC>     if (it's the one we want) then goto done
> GC>     else wait a moment, then goto loop 0
> GC>     done:
> GC> I say this because it seems...to...do...that...ever...so...slowly and
> GC> it becomes tiresome to watch--I want to shout "Hit Home! Jump to [none]!".
> 
>  The tests are definitely not as smart as [some] users and I can't
> responsibly recommend watching them for the amusement value. But they do
> currently use "Home" to switch to the first item before starting iterating
> over them, see
> 
> https://github.com/wxWidgets/wxWidgets/blob/master/src/common/uiactioncmn.cpp#L180
> 
> So what you must be seeing is how they select the requested column when
> using custom columns and this should improve dramatically with the latest
> version as there is only one column to be set now, instead of 12 before.
> 
>  But please let me know if I'm wrong and you still would like to optimize
> the test behaviour after applying this patch.

Okay, thanks, I'll take another look.