FLTK logo

STR #2757

FLTK matrix user chat room
(using Element browser app)   FLTK gitter user chat room   GitHub FLTK Project   FLTK News RSS Feed  
  FLTK Apps      FLTK Library      Forums      Links     Login 
 Home  |  Articles & FAQs  |  Bugs & Features  |  Documentation  |  Download  |  Screenshots  ]

Return to Bugs & Features | Post Text | Post File | SVN ⇄ GIT | Prev | Next ]

STR #2757

Application:FLTK Library
Status:5 - New
Priority:1 - Request for Enhancement, e.g. asking for a feature
Scope:3 - Applies to all machines and operating systems
Subsystem:Core Library
Summary:Allows shortcuts on browser items
Created By:dfatfl
Assigned To:Unassigned
Fix Version:Unassigned
Update Notification:

Receive EMails Don't Receive EMails

Trouble Report Files:

Post File ]
Name/Time/Date Filename/Size  
#1 dfatfl
01:44 Oct 29, 2011
#2 dfatfl
21:55 Dec 09, 2011

Trouble Report Comments:

Post Text ]
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...


(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:

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:

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


Comments are owned by the poster. All other content is copyright 1998-2024 by Bill Spitzak and others. This project is hosted by The FLTK Team. Please report site problems to 'erco@seriss.com'.