#14835: wxRichToolTipInfo for wxRichToolTip
14 views
Skip to first unread message
wxTrac
Nov 17, 2012, 4:34:41 AM11/17/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14835>
#14835: wxRichToolTipInfo for wxRichToolTip
---------------------------------------------+------------------------------
Reporter: johnr | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone:
Component: GUI-generic | Version: 2.9-svn
Keywords: wxRichToolTip wxRichToolTipInfo | Blockedby:
Patch: 1 | Blocking:
---------------------------------------------+------------------------------
Attached patch adds wxRichToolTipInfo as a storage class for
wxRichToolTip.
Patch also modifies the dialogs sample's RichTipDialog class.
wxRichToolTipInfo is based on discussion earlier in the year. Its main
purpose is to be a member a toplevel window or the application object to
achieve visual standardization and avoid repetitive setting of parameters
when using richtooltips.
I haven't done the help docs for it yet as I expect there will be some
changes requested on this first but working draft.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14835>
#14835: wxRichToolTipInfo for wxRichToolTip
---------------------------------------------+------------------------------
Reporter: johnr | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone:
Component: GUI-generic | Version: 2.9-svn
Keywords: wxRichToolTip wxRichToolTipInfo | Blockedby:
Patch: 1 | Blocking:
---------------------------------------------+------------------------------
Attached patch adds wxRichToolTipInfo as a storage class for
wxRichToolTip.
Patch also modifies the dialogs sample's RichTipDialog class.
wxRichToolTipInfo is based on discussion earlier in the year. Its main
purpose is to be a member a toplevel window or the application object to
achieve visual standardization and avoid repetitive setting of parameters
when using richtooltips.
I haven't done the help docs for it yet as I expect there will be some
changes requested on this first but working draft.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14835>
wxTrac
Nov 17, 2012, 6:39:04 PM11/17/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14835#comment:1>
#14835: wxRichToolTipInfo for wxRichToolTip
--------------------------------------------------------+-------------------
Patch: 1 | Blocking:
--------------------------------------------------------+-------------------
Changes (by vadz):
* keywords: wxRichToolTip wxRichToolTipInfo => wxRichToolTip
wxRichToolTipInfo needs-work
Comment:
Thanks!
I don't have any special comments about the public API changes so the
documentation can be safely written. One problem I do have with this patch
is that it does several things at once, AFAICS it
1. Adds "show delay" timeout.
1. Adds "rect" parameter in `ShowFor()`.
1. Adds `DismissFor()`.
1. Adds `wxRichToolTipInfo`.
It seems like I should be able to commit those changes independently
relatively easily so it's not a big deal but it would still be even easier
to split changes in separate patches from the beginning if possible.
I also have some comments about the non-public changes, mostly minor (and
some very minor):
1. I don't like overloads of virtual functions, this makes it impossible
to override just one of them (because this hides the other ones) and can
lead to code duplication as can be seen in wxMSW implementation here. It
would be better to use the same virtual function with some special values
of parameters (e.g. `-1` for the delay, `NULL` for the rectangle) to
indicate that they're not specified.
1. I don't think `DismissFor()` should be redeclared in the derived impl
classes, what is this for?
1. `m_timedelay` should be called `m_timeDelay` to honour our naming
convention (or maybe just `m_delay` for consistency with `m_timeout`?).
1. There is really no need for "`m_title = wxString();`" in
`wxRichToolTipInfo` ctor. Either just omit this (and the next) line(s) or
initialize them in the initializer list if you really want to. But better
just omit them. On a related note, it would be marginally better to
initialize `m_title` and `m_message` in the initializer list in the non-
default ctor.
1. It would be nice to consistently use "`if ( cond )`" style but right
now patch also has some additions of "`if( cond )`" and "`if (cond)`".
1. Comment "must be done" is not very useful. Presumably it must, yes,
otherwise why is the code there? But why? If it's obvious (it seems to be
so to me, we must store the timeout to use it later...), just omit the
comment. If it isn't, please explain why (e.g. "this is used later in
OnTimer()").
1. What is not obvious to me is why has the timer been changed to be
"multiple" instead of one shot? As we restart it anyhow, it seems
unnecessary. Wouldn't it be better to just store a flag telling us whether
we're preparing to show the tooltip or preparing to hide it? This would
get rid of the strange interval check in `OnTimer()` too and would be much
more clear IMHO.
1. There is some commented out code in `OnTimer()` which should
presumably be just removed.
1. I wonder if `DismissFor()` implementation wouldn't be simpler if we
stored a list of (normally very few) currently shown tooltips indexed by
the associated window?
1. In wxMSW code the new `ShowFor()` overload duplicates the existing
code. This would hopefully change if we have just one `ShowFor()` with
optional `rect`, as mentioned above.
1. It also should presumably check for valid rect and not use the native
implementation in this case as it doesn't support this.
1. AFAICS `DismissFor()` doesn't work with native MSW tooltips currently.
Sorry for so many remarks but, again, most of them are very minor. The
most important one is (1), then perhaps (11) and (12).
TIA!
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14835#comment:1>
#14835: wxRichToolTipInfo for wxRichToolTip
--------------------------------------------------------+-------------------
Reporter: johnr | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone:
Component: GUI-generic | Version: 2.9-svn
Keywords: wxRichToolTip wxRichToolTipInfo needs-work | Blockedby:
Type: enhancement | Status: new
Priority: normal | Milestone:
Component: GUI-generic | Version: 2.9-svn
Patch: 1 | Blocking:
--------------------------------------------------------+-------------------
Changes (by vadz):
* keywords: wxRichToolTip wxRichToolTipInfo => wxRichToolTip
wxRichToolTipInfo needs-work
Comment:
Thanks!
I don't have any special comments about the public API changes so the
documentation can be safely written. One problem I do have with this patch
is that it does several things at once, AFAICS it
1. Adds "show delay" timeout.
1. Adds "rect" parameter in `ShowFor()`.
1. Adds `DismissFor()`.
1. Adds `wxRichToolTipInfo`.
It seems like I should be able to commit those changes independently
relatively easily so it's not a big deal but it would still be even easier
to split changes in separate patches from the beginning if possible.
I also have some comments about the non-public changes, mostly minor (and
some very minor):
1. I don't like overloads of virtual functions, this makes it impossible
to override just one of them (because this hides the other ones) and can
lead to code duplication as can be seen in wxMSW implementation here. It
would be better to use the same virtual function with some special values
of parameters (e.g. `-1` for the delay, `NULL` for the rectangle) to
indicate that they're not specified.
1. I don't think `DismissFor()` should be redeclared in the derived impl
classes, what is this for?
1. `m_timedelay` should be called `m_timeDelay` to honour our naming
convention (or maybe just `m_delay` for consistency with `m_timeout`?).
1. There is really no need for "`m_title = wxString();`" in
`wxRichToolTipInfo` ctor. Either just omit this (and the next) line(s) or
initialize them in the initializer list if you really want to. But better
just omit them. On a related note, it would be marginally better to
initialize `m_title` and `m_message` in the initializer list in the non-
default ctor.
1. It would be nice to consistently use "`if ( cond )`" style but right
now patch also has some additions of "`if( cond )`" and "`if (cond)`".
1. Comment "must be done" is not very useful. Presumably it must, yes,
otherwise why is the code there? But why? If it's obvious (it seems to be
so to me, we must store the timeout to use it later...), just omit the
comment. If it isn't, please explain why (e.g. "this is used later in
OnTimer()").
1. What is not obvious to me is why has the timer been changed to be
"multiple" instead of one shot? As we restart it anyhow, it seems
unnecessary. Wouldn't it be better to just store a flag telling us whether
we're preparing to show the tooltip or preparing to hide it? This would
get rid of the strange interval check in `OnTimer()` too and would be much
more clear IMHO.
1. There is some commented out code in `OnTimer()` which should
presumably be just removed.
1. I wonder if `DismissFor()` implementation wouldn't be simpler if we
stored a list of (normally very few) currently shown tooltips indexed by
the associated window?
1. In wxMSW code the new `ShowFor()` overload duplicates the existing
code. This would hopefully change if we have just one `ShowFor()` with
optional `rect`, as mentioned above.
1. It also should presumably check for valid rect and not use the native
implementation in this case as it doesn't support this.
1. AFAICS `DismissFor()` doesn't work with native MSW tooltips currently.
Sorry for so many remarks but, again, most of them are very minor. The
most important one is (1), then perhaps (11) and (12).
TIA!
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14835#comment:1>
wxTrac
Nov 17, 2012, 11:21:40 PM11/17/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14835#comment:2>
#14835: wxRichToolTipInfo for wxRichToolTip
--------------------------------------------------------+-------------------
Yes the patch does have wide scope.
First 1-3. These were already in my wx code for other purposes but I will
refactor the patches into separate entities.
1. Point taken.
2. I did start with a different implementation and forgot to remove it.
3. See 1.
4. I always feel there will be less problems elsewhere if class members
are initialized. At present you can pass a default constructed
wxRichToolTipInfo object to wxRichToolTip and have no consequences other
than a blank tip at present. I can omit this and see how it behaves.
5. I use "if( cond )" in my own code and it is hard to change things that
are almost reflex. There is such variation through wx that I try to fit
with the existing format but point taken.
6 & 8. Some of these comments are hurried reminders to myself and will be
removed or expanded. Nothing worse than trying to follow another's poorly
commented code.
7. I wasn't keen on adding any more new members but I can do it that way.
It will be more clear.
9. It is inefficient to cycle through a window's children but I don't see
anywhere common and permanent where this could be done otherwise at first
thought. We do need to delete any existing tooltip if it is a re-show via
a mouse hover.
In mitigation:
a. The tip parent will usually be a button or control with either zero,
one tooltip child or few children.
b. Calling and enumerating a locally stored list or a GetChildren list
shouldn't differ much.
10. Can do. I wasn't sure about removing the original one.
11. I used "if ( rect.IsEmpty() )" as a validity check in SetPosition()
but for msw see 12. The rect is needed to position the tip for small
buttons and non wxWindow buttons such as found in wxRibbon. Otherwise the
tip can cover half of the button, especially for wxTipKind_None, and
clicking on the tip doesn't dismiss it.
12. I focused on the generic code to get it working and for the same
reason as not doing docs. I need to examine it more.
Not bad for a quick appraisal and I expected more remarks! Thanks for your
time spent looking at this.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14835#comment:2>
#14835: wxRichToolTipInfo for wxRichToolTip
--------------------------------------------------------+-------------------
Reporter: johnr | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone:
Component: GUI-generic | Version: 2.9-svn
Type: enhancement | Status: new
Priority: normal | Milestone:
Component: GUI-generic | Version: 2.9-svn
Keywords: wxRichToolTip wxRichToolTipInfo needs-work | Blockedby:
Patch: 1 | Blocking:
--------------------------------------------------------+-------------------
Comment(by johnr):
Patch: 1 | Blocking:
--------------------------------------------------------+-------------------
Yes the patch does have wide scope.
First 1-3. These were already in my wx code for other purposes but I will
refactor the patches into separate entities.
1. Point taken.
2. I did start with a different implementation and forgot to remove it.
3. See 1.
4. I always feel there will be less problems elsewhere if class members
are initialized. At present you can pass a default constructed
wxRichToolTipInfo object to wxRichToolTip and have no consequences other
than a blank tip at present. I can omit this and see how it behaves.
5. I use "if( cond )" in my own code and it is hard to change things that
are almost reflex. There is such variation through wx that I try to fit
with the existing format but point taken.
6 & 8. Some of these comments are hurried reminders to myself and will be
removed or expanded. Nothing worse than trying to follow another's poorly
commented code.
7. I wasn't keen on adding any more new members but I can do it that way.
It will be more clear.
9. It is inefficient to cycle through a window's children but I don't see
anywhere common and permanent where this could be done otherwise at first
thought. We do need to delete any existing tooltip if it is a re-show via
a mouse hover.
In mitigation:
a. The tip parent will usually be a button or control with either zero,
one tooltip child or few children.
b. Calling and enumerating a locally stored list or a GetChildren list
shouldn't differ much.
10. Can do. I wasn't sure about removing the original one.
11. I used "if ( rect.IsEmpty() )" as a validity check in SetPosition()
but for msw see 12. The rect is needed to position the tip for small
buttons and non wxWindow buttons such as found in wxRibbon. Otherwise the
tip can cover half of the button, especially for wxTipKind_None, and
clicking on the tip doesn't dismiss it.
12. I focused on the generic code to get it working and for the same
reason as not doing docs. I need to examine it more.
Not bad for a quick appraisal and I expected more remarks! Thanks for your
time spent looking at this.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14835#comment:2>
wxTrac
Dec 7, 2012, 9:50:49 PM12/7/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14835#comment:3>
#14835: wxRichToolTipInfo for wxRichToolTip
--------------------------------------------------------+-------------------
Accessing the patch now gives
"Trac detected an internal error:
IndexError: list index out of range"
I could possibly reproduce the bug from my 5 day old hazy memory of the
upload transaction.
I guess I will just try to upload again when I bring the patch to "apply
ready" status.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14835#comment:3>
#14835: wxRichToolTipInfo for wxRichToolTip
--------------------------------------------------------+-------------------
Reporter: johnr | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone:
Component: GUI-generic | Version: 2.9-svn
Type: enhancement | Status: new
Priority: normal | Milestone:
Component: GUI-generic | Version: 2.9-svn
Keywords: wxRichToolTip wxRichToolTipInfo needs-work | Blockedby:
Patch: 1 | Blocking:
--------------------------------------------------------+-------------------
Comment(by johnr):
I found a bug in trac here when uploading a trimmed version of the patch.
Patch: 1 | Blocking:
--------------------------------------------------------+-------------------
Comment(by johnr):
Accessing the patch now gives
"Trac detected an internal error:
IndexError: list index out of range"
I could possibly reproduce the bug from my 5 day old hazy memory of the
upload transaction.
I guess I will just try to upload again when I bring the patch to "apply
ready" status.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/14835#comment:3>
Reply all
Reply to author
Forward
0 new messages