bugGNU Octave - Bugs: bug #44822, Case-sensitive listdlg's...

 
 

bug #44822: Case-sensitive listdlg's SelectionMode values

Submitter:  Guillaume <gyom>
Submitted:  Sun 12 Apr 2015 10:33:21 PM UTC
   
 
Category:  Plotting with OpenGL Severity:  3 - Normal
Priority:  5 - Normal Item Group:  None
Status:  Fixed Assigned to:  None
Originator Name:  Guillaume Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 16 Apr 2015 04:36:05 PM UTC, comment #9: 

Pushed cset here http://hg.savannah.gnu.org/hgweb/octave/rev/8bfadb26afcd.  Marking as fixed and closing report.  Thanks for another patch Guillaume.

Rik <rik5>
Group administrator
Wed 15 Apr 2015 03:40:27 PM UTC, comment #8: 

Thanks Rik, unless I'm mistaken, the patch follows the convention you describe.

Guillaume <gyom>
Tue 14 Apr 2015 07:23:52 PM UTC, comment #7: 

The current convention is derived from jwe's dislike of CamelCase.  We should generally use strcmpi or tolower() as necessary to support arbitrary case of the input.

The Qt enumerations are something we don't have as much control over so we leave them be.

Rik <rik5>
Group administrator
Tue 14 Apr 2015 12:17:23 PM UTC, comment #6: 

Indeed, with the new syntax, property names have to be CamelCase. Given that, it seems to me that good practice in Octave would be to always use CamelCase for property names and lower case for property values in code and documentation (but accepting other formatting).

@sebald, thanks for your instructive answer, I've updated the patch with a call to tolower() and used lower case formatting in dialog.cc.

(file #33672)

Guillaume <gyom>
Tue 14 Apr 2015 07:03:58 AM UTC, comment #5: 

Just a notice: Matlab recently turned their handle type interface for graphics objects into a objects interface. This means a construct like the following:


hax = axes ();
set (hax, 'linewidth', 2)


should now be written


hax = axes ();
hax.LineWidth = 2;


I am under the impression that CamelCased property names will become a prerequisite for Matlab compatibility when we decide to switch to this object interface.

Pantxo Diribarne <pantxo>
Group Member
Mon 13 Apr 2015 06:31:03 PM UTC, comment #4: 

I'll let someone with more broad knowledge of the whole project make the call on consistency, but given uigetfile appears to have tweaked the code to first cast things to lower case, perhaps that's what should be done here as well.

There is this


          if (srole == "YesRole")
            erole = QMessageBox::YesRole;
          else if (srole == "NoRole")
            erole = QMessageBox::NoRole;
          else if (srole == "RejectRole")
            erole = QMessageBox::RejectRole;
          else if (srole == "AcceptRole")
            erole = QMessageBox::AcceptRole;


which uses CamelCase when not necessary, but I think I did that just because camel case matches the Qt enumeration definitions and noncamelcase is sometimes clumsy to read.  Also, in this case YesRole isn't something entered by the user.

Also, in the code is the following:


//  else if ()
//    view->setSelectionMode (QAbstractItemView::ContiguousSelection);
//  else if ("Multiple")
//    view->setSelectionMode (QAbstractItemView::MultiSelection);


We can delete that, given this has been used for a while and there has been no complaints about the way multiple selection works.  The only difference between ExtendedSelection and MultiSelection is that extended also allows use of the shift key for selecting everything between two entries--that is typical behavior now days.

Dan Sebald <sebald>
Mon 13 Apr 2015 03:21:14 PM UTC, comment #3: 

In MATLAB documentation, they always use CamelCase for property names (e.g. "PropertyName") and lower case for property values (e.g. "propertyvalue"), even if both of them are case-insensitive. Given this, it would make sense if all occurrences of these terms in Octave code were reflecting this convention, even if using strcmpi/Qt::CaseInsensitive. Would you agree?

That said, I'm not sure to guess which option you are preferring between adjusting the M-file or the C++ code. I noticed, for example, that uigetfile calls tolower() whenever needed (i.e. use the first discussed option).

Guillaume <gyom>
Mon 13 Apr 2015 02:54:32 AM UTC, comment #2: 

If there are more of these types of issues in the other objects of dialog.cc, they should all be taken care of at the same time.

Dan Sebald <sebald>
Mon 13 Apr 2015 02:51:04 AM UTC, comment #1: 

Which is preferred?  In one case "multiple"--in any case arrangement--would be translated to "Multiple" and sent over the link.  Or, it might be better to first just translate the user's input word to all lowercase and then perform tests from there--and use 'mode == "multiple"' in dialog.cc.  That would simplify the .m file and keep dialog.cc.

In the other case, the mode is a QString, documentation is here:

http://doc.qt.io/qt-4.8/qstring.html

It looks like instead of 'mode == "multiple"', it's a simple matter of using something like:

mode.compare (&"multiple", Qt::CaseInsensitive)


Dan Sebald <sebald>
Sun 12 Apr 2015 10:33:21 PM UTC, original submission:  

listdlg returns an error if SelectionMode is not exactly "Single" or "Multiple":


listdlg(...
  'ListString',{'aaa','bbb','ccc','ddd'},...
  'SelectionMode','multiple');


The attached patch just replaces strcmp() with strcmpi() but there is still an subsequent issue in _octave_link_list_dialog_: I feel the case mismatch should be fixed there but it might be simpler to enforce specific spelling from listdlg().

Guillaume <gyom>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #33672:  case_listdlg.diff added by gyom (2KiB - text/x-patch)
file #33654:  selectionmode_listdlg.diff added by gyom (504B - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by pantxo (Posted a comment)
  • -email is unavailable- added by sebald (Posted a comment)
  • -email is unavailable- added by gyom (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-04-16 rik5 StatusNone Fixed
        Open/ClosedOpen Closed
    2015-04-14 gyom Attached File- Added case_listdlg.diff, #33672
    2015-04-12 gyom Attached File- Added selectionmode_listdlg.diff, #33654

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code