| | [ Return to Bugs & Features | SVN ⇄ GIT ]
STR #3435
| Application: | FLTK Library |
|---|
| Status: | 1 - Closed w/Resolution |
|---|
| Priority: | 3 - Moderate, e.g. unable to compile the software |
|---|
| Scope: | 3 - Applies to all machines and operating systems |
|---|
| Subsystem: | Documentation |
|---|
| Summary: | Comment header warnings in FL/x.H and FL/mac.H contradict each other |
|---|
| Version: | 1.4-current |
|---|
| Created By: | greg.ercolano |
|---|
| Assigned To: | AlbrechtS |
|---|
| Fix Version: | 1.4.0 (SVN: v12643) |
|---|
| Update Notification: | |
|---|
Trouble Report Files:
No files
Trouble Report Comments:
| |
| #1 | greg.ercolano
13:16 Nov 14, 2017 |
| This comes up when a user application wants to use fl_mac_set_about(),
which its docs say FL/x.H needs to be included, but warnings in x.H say
never to include this file.
Currently the top of FL/x.H reads:
_____
// These are internal fltk symbols that are necessary or useful for
// calling Xlib. You should include this file if (and ONLY if) you
// need to call Xlib directly. These symbols may not exist on non-X
// systems.
_____
and the top of FL/mac.H reads:
_____
// Do not directly include this file, instead use <FL/x.H>. It will
// include this file if "__APPLE__" is defined. This is to encourage
// portability of even the system-specific code...
_____
So apparently x.H is no longer Xlib specific, it has mac oriented
stuff, and is a portability library now.
So the x.H docs should be modified accordingly.
PS. Personally, I don't like any of this; fl_mac_set_about()
is related to Fl_Sys_Menu_Bar.H, and should be defined by
including that file. Indeed it used to include x.H to take care
of that in 1.3.x and older; in 1.4.x it changed, breaking old apps
unnecessarily IMHO.
Also, x.H is a bad name for a portability file; it sounds like X11
related, as its header says it is.
Perhaps instead of forcing people to include x.H, this might be a
good time to make an os.H file instead, and recommend apps include
that. It's a better name for portability. Or name it something else,
ANYTHING else than x.H.
Even if all os.H does is include x.H, it'd hide the mess from the
public, and help prevent confusing #include files in apps going
forward. | |
| |
| #2 | AlbrechtS
03:39 Nov 15, 2017 |
| 1) +1 for fixing the misleading comments and documentation.
2) +1 for /renaming/ x.H to a better name, maybe os.H, as you suggested [1].
3) After renaming x.H to os.H [1] we'd need to add a new file x.H with '#include <FL/os.H>' [1] and some comments for compatibility reasons and document that x.H deprecated and os.H [1] should be used instead.
This way we'd have a correct "new" file os.H [1] and and "old" (deprecated) file x.H just for compatibility. The latter could then be deleted in a later version or made optional in the installation process.
Of course, the description above could also be changed to add os.H [1] with the previous contents of x.H and change x.H as described in (3). ;-)
Regarding the documentation, chapter "Operating System Issues " states:
"All programs that need to access the operating system specific interfaces must include the following header file:
#include <FL/x.H>
Despite the name, this header file will define the appropriate interface for your environment. The pages that follow describe the functionality that is provided for each operating system."
This should be updated to refer to 'os.H' [1] and could give a hint that the old, now deprecated, name was 'x.H'.
[1] Maybe a better name would be 'platform.H' because it is not only the OS that has different types and functions but the entire platform (e.g. Windows+GDI or Windows+Cygwin+X11, macOS (native) or macOS+X11, etc.). Other OS's may have different graphics drivers as well, for instance Linux may use X11 or in the future Wayland. And, last but not least, we have already 'FL/platform_types.h'. | |
| |
| #3 | AlbrechtS
04:11 Nov 15, 2017 |
| Regarding "fl_mac_set_about() is related to Fl_Sys_Menu_Bar.H, and should be defined by including that file."
I'm not sure about this: Generally we should try not to #include header files that are not necessary to compile (user) code in public headers.
Option 1: If Fl_Sys_Menu_Bar can be used w/o the platform specific headers then we shouldn't #include x.H (os.H, platform.H) in the public headers to avoid name clashes with system headers on different platforms (ISTR that Windows.H defines grp1, grp2 etc. or something like that, and X11 headers define Window, Region, etc.; I don't know what "common" symbols macOS headers define).
Then - since fl_mac_set_about() is a macOS specific function - it should be #include'd in x.H/os.H/platform.H and we should clearly document that this needs to be done by the user.
Option 2: if fl_mac_set_about() could be used w/o platform specific code on macOS and if we had a do-nothing stub like 'fl_mac_set_about() {}' on all other platforms (even better with a neutral name like 'fl_set_about()') then this could and IMO /should/ be defined in /any/ public header (likely in Fl_Sys_Menu_Bar.H) and not in x.H/os.H/platform.H. This way the user could use fl_set_about() in a platform agnostic way and this would be a no-op on platforms that don't support it (currently all platforms except macOS).
Conclusion: now, after thinking about it and writing down my thoughts, I propose to rename fl_mac_set_about() to fl_set_about() (in the public header) and to define it in any appropriate public header, but not in x.H/os.H/platform.H. The implementation for all current platforms except macOS would be an empty function, and the macOS implementation would call fl_mac_set_about() or implement the necessary functionality directly (currently in src/Fl_Sys_Menu_Bar.mm). | |
| |
| #4 | greg.ercolano
08:55 Nov 15, 2017 |
| Re: Comment #2 item (2):
I don't think we can just rename x.H, as 1.3.x docs /recommended/
to application programmers to #include it.
I'm thinking either:
a) leave it in place as "an old wart" so old code can still build, or
b) Replace it with a one line #pragma warning indicating
"this file is going away - use #include <FL/platform.h> instead"
..and leave that in place until the next major release (1.5)
This would I think minimize breakage of old apps, as often there
are non-programmer folks who try to compile old apps against the
latest version of FLTK, and come to fltk.general with the errors. | |
| |
| #5 | greg.ercolano
09:04 Nov 15, 2017 |
| Re: comment #3 regarding Fl_Sys_Menu_Bar.H:
Fl_Sys_Menu_Bar.H can be used cross platform in place of Fl_Menu_Bar,
so it should be OK for user apps to include across platforms, and it
could define the prototype for fl_mac_set_about(), which would do nothing
on the non-mac platforms.
I'm not sure changing that function's name makes sense, as likely other
platforms either won't ever have such a function, or if they did,
would likely be implemented entirely differently.
Which is why I think the _mac was included in the name.
Not to mention it would break old apps, which I think we should try
to minimize if possible, unless absolutely necessary. | |
| |
| #6 | greg.ercolano
09:20 Nov 15, 2017 |
| Oh, and:
+1 for #include <FL/platform.H>
+1 comment header changes to reference platform.H
+1 for comment 2 item (3) -- keep x.H but change its contents
to just include the new platform.H and include a #warning
or #pragma indicating the file is going away/to use platform.H
instead.
+.5 for making fl_mac_set_about() empty stubs on other platforms.
It's such an obscure, mac-specific function, I guess I don't
mind having the #ifdef MAC in my code currently required,
but I guess it's FLTK's goal to try to prevent the need for
#ifdef's like this in user apps, so I'm kinda for it.. | |
| |
| #7 | AlbrechtS
10:01 Jan 31, 2018 |
| I assigned this STR to myself and committed (incomplete) changes in svn r12640 as discussed here and in fltk.coredev, thread "RFC: make an FL/os.H file for user apps to use instead of FL/x.H".
This first step adds the file FL/platform.H with (basically) the previous contents of FL/x.H. Comments and documentation in documentation/src/*.dox have been fixed. The code compiles and works (tested only under Linux so far). FL/x.H is retained for backwards compatibility and #include(s) <FL/platform.H>.
Note: I didn't include a "deprecated" compiler warning. I believe this would be too annoying for lots of existing code. We can maybe include such a warning in 1.4.3, 1.4.4, or 1.5.0 or whenever we feel inclined to do this.
Still to do: replace all occurrences of #include <FL/x.H> or equivalent with <FL/platform.H>. I didn't want to do this all in one commit since these further changes would change too many files. | |
| |
| #8 | AlbrechtS
10:07 Jan 31, 2018 |
| Question: while I was doing the changes described above I thought about adding a minimal FL/platform.H to FLTK 1.3-current (which will then be in release 1.3.5) to enable users and FLTK devs to update their code (replace x.H with platform.H) but keep it still compiling with FLTK 1.3-svn or 1.3.5 and later. Something like:
#include <FL/x.H>
with the usual compiler guards (or not, it wouldn't matter since FL/x.H has its guards).
What do you think about this? Votes, please... | |
| |
| #9 | AlbrechtS
14:21 Jan 31, 2018 |
| Fixed in Subversion repository.
The subject of this STR "Comment header warnings in FL/x.H and FL/mac.H contradict each other" is now fixed in svn (r12641).
I'm not sure what to do about the fl_mac_set_about() issue. I agree that we don't need to rename it, and if we eventually needed a similar function on another platform we can add fl_set_about() as a cross platform wrapper function. Hence I wouldn't add empty stubs for other platforms now. If this is okay I think the STR can be closed.
Greg, others? Comments welcome... | |
| |
| #10 | greg.ercolano
14:43 Jan 31, 2018 |
| Re: Comment #8: I like the idea of helping 1.3 apps move to 1.4 by
including a small platform.H file. Not sure I see how it could hurt.
Haven't tried the code or looked at the commits; I'm in a world
of circuit design, so currently my mind is far away from software
at the moment.. :/ | |
| |
| #11 | AlbrechtS
15:35 Jan 31, 2018 |
| Update: there were two missing changes that broke the builds under macOS and Windows so I had to fix this in svn r12642. svn r12643 fixes the same issue in FL/porting.H although this file is not used (a potential template from the time we used lots of #ifdef's before we switched to the full platform driver solution) and hopelessly outdated, but anyway, it's fixed as well. | |
| |
| #12 | AlbrechtS
10:12 Apr 19, 2018 |
| Progress report: I added FL/platform.H in FLTK 1.3 (svn r12861, branch-1.3) which will be in FLTK 1.3.5 for compatibility with FLTK 1.4.0 and higher.
I believe the main issue of this STR is resolved (documentation and inconsistencies with FL/x.H, FL/platform.H). All occurrences of FL/x.H (except those in documentation) have also been fixed/replaced.
If the issue concerning fl_mac_set_about() needs further action a new STR should be opened. If you (Greg) agree I suggest to close this STR.
Can we close it now? | |
| |
| #13 | greg.ercolano
10:32 Apr 19, 2018 |
| Sounds good.
Should we add a #pragma message warning in 1.4.0's x.H file
recommending to use platform.H instead?
I think it'd help app developers, as they're not likely to read
the comment warnings of of x.H | |
| |
| #14 | AlbrechtS
01:58 Apr 20, 2018 |
| See my comment #7: "I believe this would be too annoying for lots of existing code. We can maybe include such a warning in 1.4.3, 1.4.4, or 1.5.0 or whenever we feel inclined to do this."
Additionally, changed code would not compile with 1.3.x (x < 5, and 1.3.5 is not yet released), hence we have a compatibility problem and users would have to add #if conditionals to make their code work in both FLTK branches. Let's wait a year or two. Maybe the best would be to use the latest release of 1.4.x before we switch to 1.5.0. | |
| |
| #15 | AlbrechtS
02:27 Apr 20, 2018 |
| I committed a documentation change to prepare users of 1.3.5 for the name change so they can use FL/platform.H if they target FLTK 1.3.5 or higher.
This is in branch-1.3, svn r12864. | |
| |
| #16 | AlbrechtS
08:47 Jan 19, 2019 |
| I'm closing this STR now. The main points (see subject/summary) have been resolved a long time ago.
The only missing piece is the documentation and inclusion of the best (correct?) headers to use fl_mac_set_about(). It's still declared in FL/mac.H, but the comment in the header has been fixed.
If you still think we should change anything related to fl_mac_set_about() please open a new STR with a new subject. Optionally refer to this one, or summarize the discussion regarding fl_mac_set_about(). | |
[ Return to Bugs & Features ]
|
| |
