#15060 closed defect (fixed)

wxRichToolTip::ShowFor() rectangle inconsistencies

Reported by: lpoujoulat Owned by:
Priority: low Milestone: 3.0.0
Component: GUI-generic Version: stable-latest
Keywords: Cc: johnr
Blocked By: Blocking:
Patch: yes

Description

I noticed two strangeness with the new (nice) wxRect parameter of ShowFor().

  • Why the wxRect parameter is not const ?
  • Why the coordinates of the rectangle are screen coordinates ? I'd rather expect them to be window coordinates relative to the given window parameter ... If it's "by design" it should be stated clearly in the documentation.

As usual, I'll gladly make a patch after having your ideas on the matter

Regards,

Laurent

Attachments (2)

richtooltip_const.patch download (3.7 KB) - added by lpoujoulat 18 months ago.
richtooltip_winrect.patch download (1.3 KB) - added by lpoujoulat 18 months ago.

Download all attachments as: .zip

Change History (13)

comment:1 follow-up: Changed 18 months ago by vadz

  • Cc johnr added
  • Priority changed from normal to low
  • Status changed from new to confirmed

The rect parameter should definitely be const, this is clearly just an oversight.

As for the coordinates, I agree that using client coordinates of win would make sense to me too but I might be missing something here.

John, what do you think?

comment:2 Changed 18 months ago by vadz

  • Milestone set to 3.0

P.S. We should either change this or document the existing behaviour before 3.0.

comment:3 in reply to: ↑ 1 ; follow-up: Changed 18 months ago by johnr

Replying to vadz:

The rect parameter should definitely be const, this is clearly just an oversight.

The parameter is wxRect *rect=NULL. As I recall we used a wxRect pointer rather than a constant reference to avoid overloading ShowFor(). Do we usually make pointer parameters constant?

As for the coordinates, I agree that using client coordinates of win would make sense to me.

In retrospect, I agree. I haven't looked at the code for unsigned ints but we would need to ensure negative coordinates are catered for.

comment:4 in reply to: ↑ 3 ; follow-up: Changed 18 months ago by lpoujoulat

Replying to johnr:

Replying to vadz:

The rect parameter should definitely be const, this is clearly just an oversight.

The parameter is wxRect *rect=NULL. As I recall we used a wxRect pointer rather than a constant reference to avoid overloading ShowFor(). Do we usually make pointer parameters constant?

In my point of view, making a pointer constant is not a problem, but overloading would probably be cleaner and more secure, although this would break any code using this feature ... so it's up to you :-)

As for the coordinates, I agree that using client coordinates of win would make sense to me.

In retrospect, I agree. I haven't looked at the code for unsigned ints but we would need to ensure negative coordinates are catered for.

I didn't see anything preventing to use negative coordinates if we go for win coords.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 18 months ago by johnr

Replying to lpoujoulat:

In my point of view, making a pointer constant is not a problem, but overloading would probably be cleaner and more secure, although this would break any code using this feature ... so it's up to you :-)

There are some relevant comments in #14835 made about the original rough draft. A pity I lost the original patch but I have the other components in my working copy. I have no problem with, and prefer, making the parameter const.

I didn't see anything preventing to use negative coordinates if we go for win coords.

The advantage is that we don't have to use rect.Offset( wxPoint( GetScreenPosition().x, GetScreenPosition().y in out own code. This will break some code and would not be good if it was a core feature but I wonder whether many are using this parameter.

comment:6 in reply to: ↑ 5 Changed 18 months ago by lpoujoulat

Replying to johnr:

Replying to lpoujoulat:

In my point of view, making a pointer constant is not a problem, but overloading would probably be cleaner and more secure, although this would break any code using this feature ... so it's up to you :-)

There are some relevant comments in #14835 made about the original rough draft. A pity I lost the original patch but I have the other components in my working copy. I have no problem with, and prefer, making the parameter const.

I didn't see anything preventing to use negative coordinates if we go for win coords.

The advantage is that we don't have to use rect.Offset( wxPoint( GetScreenPosition().x, GetScreenPosition().y in out own code. This will break some code and would not be good if it was a core feature but I wonder whether many are using this parameter.

I don't think the API change is a problem as it is a new one, not present in 2.9.4

So to conclude, I have to make a patch that:

  • make the rectangle pointer "const" (not using reference)
  • change ShowFor() to use window coordinates
  • Update the documentation to be clear about the kind of coordinates that are used

Did I forgot something ?

Changed 18 months ago by lpoujoulat

comment:7 Changed 18 months ago by lpoujoulat

  • Patch set

I've just add a first patch that take care of the "const" issue.
I'll make the next one for window coordinates tomorrow.

Changed 18 months ago by lpoujoulat

comment:8 follow-up: Changed 18 months ago by lpoujoulat

And here is the patch to use window coordinates instead of screen coordinates

Regards

comment:9 in reply to: ↑ 8 Changed 18 months ago by johnr

Replying to lpoujoulat:
Thanks, nothing forgotten as there was no sample example to update. Looks good to me.

comment:10 Changed 18 months ago by VZ

(In [73589]) Make wxRect parameter of wxRichToolTip::ShowFor() const.

This parameter is read-only, so accept a const pointer here.

See #15060.

comment:11 Changed 18 months ago by VZ

  • Resolution set to fixed
  • Status changed from confirmed to closed

(In [73590]) Interpret wxRect passed to wxRichToolTip::ShowFor() as client coords.

It makes more sense to use the window coordinates here instead of the screen
ones.

Closes #15060.

Note: See TracTickets for help on using tickets.