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.
|
Wed 15 Apr 2015 03:40:27 PM UTC, comment #8:
Thanks Rik, unless I'm mistaken, the patch follows the convention you describe.
|
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.
|
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)
|
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:
should now be written
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.
|
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
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:
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.
|
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).
|
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.
|
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)
|
Sun 12 Apr 2015 10:33:21 PM UTC, original submission:
listdlg returns an error if SelectionMode is not exactly "Single" or "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().
|