| | [ Return to Bugs & Features | Roadmap 1.3 | SVN ⇄ GIT ]
STR #2948
| Application: | FLTK Library |
|---|
| Status: | 1 - Closed w/Resolution |
|---|
| Priority: | 1 - Request for Enhancement, e.g. asking for a feature |
|---|
| Scope: | 3 - Applies to all machines and operating systems |
|---|
| Subsystem: | Core Library |
|---|
| Summary: | Add a method to Fl_Widget that returns its top-level window |
|---|
| Version: | 1.3-feature |
|---|
| Created By: | greg.ercolano |
|---|
| Assigned To: | greg.ercolano |
|---|
| Fix Version: | 1.3-current (SVN: v9876) |
|---|
| Fix Commit: | 5fda885c0306561ab53266e16d87b3c4e164572c |
|---|
| Update Notification: | |
|---|
Trouble Report Files:
Trouble Report Comments:
| |
| #1 | greg.ercolano
12:05 Apr 09, 2013 |
| This STR spawned from a discussion on fltk.development,
Subject: "RFC: method to find top level window?"
The suggestion is to add a method that returns the top-level window
for the current widget, i.e. the 'window manager window' the widget
is in.
This differs from Fl_Widget::window() which may return a sub-window,
if there is one, not necessarily the top-level window.
I suggested "toplevel_window()" because the term 'top-level' seems
to be used in the code comments, Albrecht offered "top_window()"
as a shorter alternative, which I think I'll go with here.
Attaching a patch suggesting the code implementation,
"toplevel_window.patch".
Also attaching a test program, "test-top_window.cxx"
which compares the return value of window() and top_window() for
various widgets and windows in the presence of sub-windows.
We should also probably do a code check on all of FLTK
to make sure "window()" isn't being used where top_window()
would be correct. I believe it's only recently that FLTK reliably
supported sub-windows, so the chances of apps using them are
higher now, and little problems are more likely to spring up,
such as STR #2944 [2].
Comments welcome. | |
| |
| #2 | manolo
03:45 Apr 10, 2013 |
| - The last statement of the Fl_Widget::top_window() function
may be written more simply:
return w->as_window();
- I'm unsure this function would be useful. Can the knowledge of
the top enclosing window of an object be useful without knowledge of
the object coordinates relatively to this window?
May be what is needed would be along this line:
Fl_Window* Fl_Widget::top_window_offset(int& xoff, int& yoff) | |
| |
| #3 | greg.ercolano
08:35 Apr 10, 2013 |
| > Can the knowledge of the top enclosing window of an object
> be useful without knowledge of the object coordinates
> relatively to this window?
Simple case would be a widget (say a button) that wants to hide
the window its in. If the button is in a subwindow, window() wouldn't
work, bot top_window() would.
> May be what is needed would be along this line:
> Fl_Window* Fl_Widget::top_window_offset(int& xoff, int& yoff)
Actually just added such a thing in r9866 a few days ago to solve
STR#2944, but called it window_offset(). Perhaps I should add "top_"
to it though.. its not too late. | |
| |
| #4 | AlbrechtS
11:38 Apr 10, 2013 |
| Patch looks good, except what Manolo wrote already. We should remove this old and error-prone "w->type() >= FL_WINDOW" from the entire lib in favor of as_window().
WRT window_offset() vs. top_window_offset(): I strongly vote for the latter and consistent naming, i.e. top_window() and top_window_offset().
+1 for the patch with mods as written. | |
| |
| #5 | greg.ercolano
13:00 Apr 10, 2013 |
| > as_window()
Yes, that looks good.. I'll do some tests; want to make sure
it inherits through all the window options we have (Fl_Gl_Window, etc)
> window_offset() vs. top_window_offset(): I strongly vote for
> the latter and consistent naming
Agreed.
Will make changes and implement.
> We should remove this old and error-prone "w->type() >= FL_WINDOW"
> from the entire lib in favor of as_window().
OK, I'll try to handle that too, but as a separate commit. | |
| |
| #6 | greg.ercolano
13:39 Apr 10, 2013 |
| Oh, BTW, I think I missed what Manolo might have been getting at
wrt the 'top_window_offset()' method.
So where we're at now, just to disambiguate:
o There's currently a method Fl_Window::top_window_offset()
(just renamed it from window_offset() in r9870)
o We're proposing in this STR a new Fl_Widget::top_window().
I can see where the existing top_window_offset() should really
be a method of Fl_Widget, not just Fl_Window.. so that even
non-windows can find their offset relative to the top window
to be more useful.
So will look into changing that as well. | |
| |
| #7 | greg.ercolano
14:16 Apr 10, 2013 |
| OK:
r9871: top_window(): implemented (using as_window())
r9872: top_window_offset() now a member of Fl_Widget (was Fl_Window)
and moved it near top_window() and window(). | |
| |
| #8 | greg.ercolano
14:22 Apr 10, 2013 |
| ..and finally:
r9873: CHANGES file updated.
Comments before close? | |
| |
| #9 | AlbrechtS
14:27 Apr 10, 2013 |
| Comments? Yes: Great, thanks.
BTW: good catch that top_window_offset() should be a Fl_Widget method. I didn't realize that it was Fl_Window:: before. | |
| |
| #10 | manolo
03:41 Apr 11, 2013 |
| Thanks Greg for implementing exactly what I was getting at.
I still have a few remarks about the current implementation of:
Fl_Window* Fl_Widget::top_window_offset(int&, int&)
- The doc should say 'current widget' instead of 'current window'
because a widget is now current.
- I feel the widget-to-window cast
const Fl_Window *win = (const Fl_Window *)this;
is frightening, because widgets are not windows, in general.
It works here because only member functions from the parent Fl_Widget
class are employed. Rewriting it
Fl_Widget *win = (Fl_Widget*)this;
does just the same, removing the fear.
- This implementation applied to a window-less widget returns
this widget cast as a window, a potentially dangerous thing.
My suggestion would be to use (again) our friend Fl_Widget::as_window().
The attached window.patch gathers all of this. | |
| |
| #11 | greg.ercolano
10:59 Apr 11, 2013 |
| @manolo: Great -- agree with all, except perhaps loosing const here:
- const Fl_Window *win = (const Fl_Window*)this;
+ Fl_Widget *win = (Fl_Widget*)this;
I think we can maintain const protection on the variable this way:
const Fl_Widget *win = this;
:
return ((Fl_Widget*)win)->as_window();
..this ensures const protection for all uses of win within the code,
and we only turn it off when we need to (eg. the non-const as_window() call)
Also, I should probably change all instances of 'win' -> 'w' in the
code, since it's really working with widgets now.
Thanks for the peer review -- will hold on comments regarding const
above, as am curious if there's a reason not to. | |
| |
| #12 | AlbrechtS
02:45 Apr 12, 2013 |
| +1 for renaming to w instead of win (or maybe widget to avoid name clashes with w(), but that's not important here, IMO)
+0 for maintaining constness (yes, I mean + zero). constness doesn't gain much here, and since compilers become pickier more and more, there is the danger of getting a warning when it's cast away for as_window(), as Ian wrote elsewhere. If not now, then maybe later. We're only calling const methods anyway, as x(), y(), window(), and this code is not subject to be changed in the future. Using const here would IMHO only protect ourselves from adding code that would modify the widget in question, and as said before, we won't add more code. Just my 2 (€)ct. | |
| |
| #13 | manolo
03:41 Apr 12, 2013 |
| @Greg: Yes to all proposed changes.
There's no reason, but laziness, to loose const protection. | |
| |
| #14 | greg.ercolano
09:57 Apr 12, 2013 |
| Albrecht> We're only calling const methods anyway, as x(), y(), window(),
Albrecht> and this code is not subject to be changed in the future.
Yes, though for some reason "as_window()" is not const,
(it probably should be), which is why that cheat is necessary at the end there. Maybe using const_cast<Fl_Widget*>(w)->as_window() is better,
which is what I decided to go with in top_window_offset().. will check.
Note in manolo's implementation, const is cast away at the top
at the declaration of the variable, but I just switched it to const
and cast const away only at the place where it was needed, at the
as_window() call.
So this does two things:
1) Keeps the variable const so one can't later add code like
w->label("yoo hoo"); without getting an error..
hey, it could be more subtle ;)
2) Highlights exactly where the cheat is needed (at as_window())
so if as_window() ever changes in the future to be const, it's
easier to find and remove the const cast-away.
Is there a reason as_window() and friends (other virtual
implementations) are not const methods? Fl_Window::window() is. | |
| |
| #15 | greg.ercolano
16:11 Apr 12, 2013 |
| Updated fixes in r9875 and r9876.
About to close, unless there's more to add.
Regarding const mods to at_window() and friends,
let's continue that on fltk.dev and make a separate STR for that. | |
| |
| #16 | greg.ercolano
16:15 Apr 12, 2013 |
| Oh, I should add..
all mods to date (r9876, including static_cast<>) build OK on:
redhat9, irix6.5, centos 5.6, Mountain Lion default compiler (picky),
Sci Linux 6 (very picky, latest linux I have) gcc 4.4.6, and VS 2010. | |
| |
| #17 | greg.ercolano
16:21 Apr 12, 2013 |
| Sorry, as Ian points out on fltk.dev, meant to say const_cast<>
in the above, not static_cast<>. | |
| |
| #18 | greg.ercolano
12:10 Oct 27, 2014 |
| Fixed in Subversion repository. | |
| |
| #19 | AlbrechtS
19:05 Jan 15, 2023 |
| Add git commit info: 5fda885c0306561ab53266e16d87b3c4e164572c for svn r9876. | |
[ Return to Bugs & Features ]
|
| |
