test/icon demo could use some improvements

[erco77]

I found the test/icon demo a bit confusing.

I've never actually used it and couldn't really tell what it did, even from the code comments; Is this for testing widget icons or the window manager icon? When it opens it just says "Colour" and the choice is "red" but nothing is red, and what's it the color of? It's only when you change the choice to something else it properly updates the icon, which is weird. And shouldn't there be a way to set 'no icon'?

Here's a few improvements that come to mind:

1. Change the choice label from just "Colour" to "Window icon:"
2. Have "none" as the default/initial choice, and tweak the callback to allow for the 'none' choice to invoke win->icon(0);, which according to the docs is a valid way to 'reset' the icon.. a good thing to test to make sure an icon can be assigned, then unassigned.
3. The class instance Fl_RGB_Image icon goes out of scope on returning from the callback. It is not clear in the docs for Fl_Window::icon(Fl_RGB_Image) that the buffer is no longer needed, so not sure if this is a bug or correct behavior. (a) Docs for icon() should be improved me thinks, to indicate if the buffer needs to remain allocated or not, and if it is OK, (b) adding a comment on the win->icon(&icon); line that says something like // once assigned, 'icon' can go out of scope just to make it clear.
4. Perhaps a tooltip for the choice that elaborates on what this program does; affects the window manager's title bar and toolbar icon.

[erco77]

Attaching a patch suggestion, comments welcome.
icon-rfe-v1.1.txt

[erco77]

Attaching a documentation improvement suggestion to clarify that calling Fl_Window::icon(Fl_RGB_Image*) makes an internal copy of the pixel buffer (from what I can tell from the internals, this is true), so the Fl_RGB_Image instance can apparently be freed by the caller once set. docs-rfe-v1.0.txt

Similar methods (e.g. icons()) should probably also be updated. Perhaps those more familiar with the internals can weigh in on this.

[Albrecht-S]

Great enhancements, but I suggest to make the initialization simpler and easier to understand:

   window.show(argc,argv);
+  choice.do_callback();         // make default take effect
   return Fl::run();

Replace choice.do_callback(); with window->icon(0); or

  window->icon((Fl_RGB_Image*)0);          // reset window icon

(if the cast is necessary). OTOH "no icon" would be the default anyway, so why bother and set "no icon" explicitly?

You could also mention that the icon used in Alt+Tab is changed as well.

Another info (for the docs) would be that some window managers don't set a window (i.e. title bar) icon, notably Cinnamon (which I am using), although the taskbar and Alt+Tab icon is set.

[erco77]

why bother and set "no icon" explicitly?

Mainly for consistency, these reasons:

1. Ensures what the chooser currently displays as the current selection represents code in the callback was actually executed for that choice, even if it's the default.
2. Future-proofs changes to the callback code; any additions to the callback for sure will be reflected on startup.
3. Avoids duplicate code in main() (e.g. setting win->icon(0))

You could also mention that the icon used in Alt+Tab is changed as well.

Sure, I'll add that to the small list.

I was trying to avoid too many window manager specifics, as they seem to change with the wind.

Originally I was just going to say "Sets the application's window manager icon" and leave it to the reader as to what that infers, but figured a few generic examples could only help.

[erco77]

My commit apparently auto-closed this, but followup comments still welcome.

[Albrecht-S]

"Fixes #nnn" triggers the auto-close feature. Per convention this should be put at the very end of the commit message body.

Comments: none. All good. Thanks. ;-)

[erco77]

Great.

Per convention this should be put at the very end of the commit message body.

Good to know.
But the CMP needs to be updated then, as I was following the example that's there now:

    Issue Management: Fixing An Issue
    When you've applied a fix to Git, be sure to update the issue:

        Commit the fix, and reference the issue#, e.g.
        git commit -m "Fixes #92, added -d debug flag to fluid"

There's a comment below that which reads: TBD: Albrecht to elaborate, so perhaps now's the time ;-)

[Albrecht-S]

Yes indeed. Got me ;-)

Thanks for the reminder.