Code Review for Willard: BOR Truckee task 1.5 -- Slot Usability Enhancement
- Phil Weinstein, 3-20-2017 (b)

Willard, really great work of limiting changes to exactly what needs to change. This really helps the commit history/diffs to document exactly what was done to implement the desired functionality.

Also, good level of commenting. Not too much. Kinda perfect.

Note: I'll mark my comments with "Academic" to indicate my preferences, but not something that actually needs to be changed.

Reviewed, "SlotUE" branch:
  https://cadswes2.colorado.edu/internal/cgi-bin/gitweb/gitweb.pl/builds.git/shortlog/refs/heads/SlotUE

(1) Q3GUI/ScalarSlotDlg.cpp
and Q3GUI/SlotQtDlg.cpp

New code:

if ( exists && _slot )
{
   // setToggleRelevant() updates the toggle, which sets the tooltip
   // to the "null string", so we need only set an existing tooltip
   // if setting the toggle as relevant
   QString desc = _slot->getUserDesc();
   _descToggles->setToggleToolTip( _descTogglesIndex_desc, desc );
 }

Academic: I prefer to have a reasonable effect in "non-conforming" cases, rather than having no effect. If _slot is NULL, it would be better to NOT leave any possibly present previously set tooltip. (This is probably academic because if _slot changes from being defined to being NULL, the whole dialog would be deleted. If this was a more fundamental effect -- i.e. not just tooltip text -- I wouldn't want to make that assumption).

That is, I would code it like this::

QString desc = _slot ? _slot->getUserDesc() : QString("");
_descToggles->setToggleToolTip( _descTogglesIndex_desc, desc );

(2) QtRpl/RowOToggles.hpp / .cpp

Good Stuff:

• Good level of commenting. (Not too much, or too little).
• I like that you have separate rwAssert expressions on the three different conditions you are testing in RowOToggles::setToggleToolTip(), as opposed to a conjunction.

Academic:

• I do prefer (and I always do) pass non-mutable strings into methods as const references, e.g. the 2nd form:
  1. void setToggleToolTip( int toggleIndex, QString toolTip );
  2. void setToggleToolTip( int toggleIndex, const QString& toolTip );
      
• Believe it or not, I prefer to use a default constructed string instead of an empty (but non-Null) string. This used to be important in Qt, until they removed the distinction. I still prefer the 2nd form, but maybe we can see what the group thinks. (I do concede that the empty string is more readable):
  1. checkBox->setToolTip( "" );
  2. checkBox->setToolTip( QString() );
      
• I see that, in this module, we do sometimes check for the 'checkbox' pointer being NULL. That's my preference. So, I would do that also in new code, RowOToggles::setToggleToolTip().
  

Do Change:

• "size_t" is a mistake in C/C++. It's unsigned, and any integer you would potentially do subtraction on really wants to be signed. (Making such an integer "unsigned" doesn't do anything to guarantee that it won't become negative. It just guarantees that, if it does, the result is absolutely terrible). Qt4 made this transition away from "size_t" for this reason, e.g. in the collection classes. Note that QString::size_type is an int (signed). Here, this applies to the static const size_t TOOLTIP_MAX_CHARS definition in the header file.
   
• [Maybe we can discuss with David -- he might prefer what you actually did here] ... I do understand your documented rationale for defining TOOLTIP_MAX_CHARS to be as large as you have it (239) -- handily fitting within the width of a typical font on a 1920-pixel wide screen. Still though, a tooltip like that poping up feels jarring to me. I'd prefer the original idea of 140 characters max -- or, personally, even fewer, actually. But 140 is prudent. (And I love the humor of using that number).

(3) QtRpl/RplBlockDlg.cpp, QtRpl/RplFunctionDlg.cpp, QtRpl/RplGroupDlg.cpp, QtRpl/RplSetDlg.cpp

Do Change:

•  These implementations are not limiting the number of characters (i.e. to 239 or 140) .
  •  Note that these RPL dialog classes are all derived from RplBaseDlg. So that's a fine place to put a 'protected' TOOLTIP_MAX_CHARS definition. (It's OK for these dialogs to have their own such definition, instead of making this global enough to share also with the slot dialogs. But we might as well use one common definition for these RPL dialogs, because that it easy to do).  ... My mistake. This is applied at the RowOToggles level. --Phil
     
• In the RplSetDlg implementation are you refraining from setting the tooltip if the description is empty. In the others, you are unconditionally setting it. I think the latter is preferred. (Similar to my comment in section (1)).

--- (end) ---