Re: [lmi] [PATCH] wx_test_calculation_summary.cpp

[Vadim Zeitlin]

 Hello again,

 I'm attaching the patch implementing the revised specification for this
test case. 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. 


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.

 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.


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, apparently just knowing about
the Ctrl-C/Ctrl-V key combinations 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.

GC> does that OS force you to step through all choices when you already know
GC> which one you want? But see below before answering this, as I might just
GC> be misunderstanding what I think I'm seeing.

 Under OS X you need to do the following to select the first item in a
combobox:

0. You need to ensure that "full keyboard access" is on, it is off by
   default meaning that Tab only switches between text controls and buttons
   and comboboxes (as well as checkboxes, ...) can't be used from keyboard
   at all. Luckily, this can be done from the keyboard, using the standard
   Ctrl-F7 shortcut. Unluckily, I don't think there is any way to know if
   it is currently enabled or disabled, so the program would need to do
   something like: try Tab-bing to the combobox, check if it worked,
   simulate Ctrl-F7, try Tab-bing again.

1. You need to press the down arrow to open the combobox dropdown.

2. You can indeed press Home to go to the first selected item.

3. But you also need to press Enter to close the combobox, otherwise the
   selection wouldn't be taken into account.

 This is not really bad (except for the first item), but it is quite
different from the keys you need to use to perform the same task under MSW.

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.

 Thanks in advance,
VZ

0001-Implement-the-revised-calculation-summary-unit-test-.patch
Description: Text document