[ Return to Bugs & Features | Post Text | Post File | Prev | Next ]

STR #2757

Trouble Report Files:

	Name/Time/Date	Filename/Size
	#1  dfatfl 01:44 Oct 29, 2011	browser-shortcut-items.patch 3k
	#2  dfatfl 21:55 Dec 09, 2011	browser-shortcut-items-2.patch 4k

Trouble Report Comments:

	Name/Time/Date	Text
	#1  dfatfl 01:44 Oct 29, 2011	Option to enable &shortcuts on items added to browsers.  Note that this patch incorporates str2756, in that it references FL_CHECKBOX_BROWSER.  If you want to test without str2756, then remove the check for that browser type() in the if statement.
	#2  dfatfl 21:56 Dec 09, 2011	Just uploaded new version of patch which adds some additional logic options (now defaults to no selection on alt- key to allow alt- key to select duplicate shortcuts outside of browser), however option can be enabled to make alt- key work to select browser items as well.
	#3  AlbrechtS 05:31 Dec 10, 2011	David, thanks for your continued effort to post patches to improve FLTK. I looked at this one, and there are some points I want to comment on to make it easier for the FLTK team to accept your patches. My comments here are more of a general nature, less specific to this particular patch. (1) Please take a look at FLTK's CMP to see what coding style and conventions we use. Although there are some deviations, this is what the coding style *should* be. Note that I don't say that you didn't do it correctly (especially indenting, tabs, brackets etc.), but just in case you were not aware of it... http://www.fltk.org/cmp.php http://www.fltk.org/cmp.php#CODING_STANDARDS (2) Method names in FLTK use some naming conventions that your patches should also use. Especially the simple set and get methods do always use the same name, with different arguments - so... void enable_item_shortcuts() and void disable_item_shortcuts() should be void item_shortcuts(int) where the int argument is 0 or 1 to disable or enable item shortcuts, resp.. bool using_item_shortcuts() would then be int item_shortcuts() (we're always using int instead of bool for historical reasons, however this may change in the future). (3) It is good that you wrote documentations for your new methods, but it would be even better if you used normal upper case letters where appropriate (e.g. at the beginning of a sentence). (4) I'm not a fan of single-character variable names, particularly if the name is 'l' (lower case L) that can easily be confused with 1 (the digit). Examples from your patch: test_shortcut(item_text(l),... if ((l=item_next(l))==NULL) { ... The latter (4) is my personal opinion, but I believe that the code would be much better maintainable, if such variable names were avoided. (Please don't say "But they are also elsewhere in the code..." - that's true, but I would ask you to avoid this in new code, nevertheless...). Also, please note that most of your RFC's will probably be moved (temporarily ?) to 1.4-feature to prepare for upcoming 1.3.x releases. Depending on the FLTK 3 development progress, they might even be postponed to FLTK 3.0 or later, because we currently have to manage two different branches, and double code maintenance should be avoided as much as possible.
	#4  AlbrechtS 03:34 Feb 07, 2019	Moved from 1.3-feature to 1.4-feature. This STR has been superseded, see also cumulative patch in STR 3485: https://www.fltk.org/str.php?L3485

[ Return to Bugs & Features | Post Text | Post File ]