wxRichToolTipInfo for wxRichToolTip

[johnr]

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.

[vadz]

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.
2. Adds "rect" parameter in ShowFor().
3. Adds DismissFor().
4. 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.
2. I don't think DismissFor() should be redeclared in the derived impl classes, what is this for?
3. m_timedelay should be called m_timeDelay to honour our naming convention (or maybe just m_delay for consistency with m_timeout?).
4. 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.
5. It would be nice to consistently use "if ( cond )" style but right now patch also has some additions of "if( cond )" and "if (cond)".
6. 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()").
7. 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.
8. There is some commented out code in OnTimer() which should presumably be just removed.
9. 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?
10. 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.
11. It also should presumably check for valid rect and not use the native implementation in this case as it doesn't support this.
12. 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!

[johnr]

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:

1. The tip parent will usually be a button or control with either zero, one tooltip child or few children.
2. 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.

[johnr]

I found a bug in trac here when uploading a trimmed version of the patch. 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.