Opened 4 years ago

Closed 21 months ago

#13045 closed defect (fixed)

[patch] wxNotebook::HitTest() not implemented in wxOSX/Cocoa

Reported by: dhyams Owned by:
Priority: normal Milestone:
Component: wxOSX-Cocoa Version: stable-latest
Keywords: hittest osx wxnotebook simple Cc: dhyams
Blocked By: Blocking:
Patch: yes

Description

wxWidgets 2.9.1, OSX carbon (perhaps cocoa too, not sure)

Under OSX, theHitTest() routine always returns -1 (not found) no matter what. The routine has been complete disabled with an #if 0 (see src/osx/notebook.cpp). Here is a link to that code:

http://trac.wxwidgets.org/browser/wxWidgets/trunk/src/osx/notebook_osx.cpp#L248

If I just reenable the routine by changing the #if 0 to an #if 1, and remove the WXUNUSED macros, everything works fine again.

So is there a reason HitTest() has been completely removed, or was it an accident? It seems, so far, that it works perfectly well under carbon, so the code should at least stay intact for any carbon build. I can't vouch for cocoa.

Attachments (1)

cocoa_notebook_hit_test.patch download (2.3 KB) - added by dhyams 22 months ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 4 years ago by disc

  • Component changed from wxOSX-Carbon to wxOSX-Cocoa
  • Milestone changed from 2.9.2 to 2.9.3
  • Status changed from new to confirmed

I will restore the code for Carbon (which only reports wxBK_HITTEST_NOWHERE or wxBK_HITTEST_ONLABEL btw, same as in 2.8). I don't know how to fix this for Cocoa so I'm leaving the ticket open and making it Cocoa specific.

comment:2 Changed 4 years ago by DS

(In [67679]) Restored wxNotebook::HitTest for wxOSX-Carbon.

Since the copying of src/osx/carbon/notebmac.cpp to src/osx/notebook_osx.cpp in r55202 the code in wxNotebook::HitTest has been disabled. Enabled it again for at least the Carbon build.

See #13045.

comment:3 Changed 3 years ago by vadz

  • Milestone 2.9.3 deleted
  • Summary changed from HitTest() does not work with wxNotebooks to HitTest() does not work with wxNotebooks in wxOSX/Cocoa

comment:4 Changed 2 years ago by vadz

  • Keywords simple added
  • Summary changed from HitTest() does not work with wxNotebooks in wxOSX/Cocoa to wxNotebook::HitTest() not implemented in wxOSX/Cocoa
  • Version changed from 2.9.1 to 2.9-svn

It looks like it would be pretty simple to implement this using NSTabView::tabViewItemAtPoint.

Any takers?

comment:5 Changed 22 months ago by dhyams

  • Cc dhyams added

I'm trying, but apparently I don't know how things fit together. I wrote a small bit of code that implements HitTest for Cocoa (using NSTabView::tabViewItemAtPoint), and put it in src/osx/cocoa/notebook.mm.

Also effectively commented out HitTest over in src/osx/notebook_osx.cpp by moving the carbon #ifdefs.

Then compiled, install wx, and ran a test app. My HitTest is not being called. I'm just not sure how the pieces are all supposed to fit together...I'm not an objective C programmer, nor Mac programmer, nor wx programmer.

If I post the bit of code that implements HitTest, can someone teach me where to put it so that it will be called? I'm pretty sure that is either correct or close to correct.

comment:6 Changed 22 months ago by vadz

Just for the reference, this is also discussed in this thread.

Changed 22 months ago by dhyams

comment:7 Changed 22 months ago by dhyams

OK, this is done. As for the refactoring suggested in the thread referenced one post up, I wasn't really comfortable doing either...best left to the wx pros here I think.

This patch Works For Me (TM); tested and the hit testing is working. I attached the patch onto this ticket.

The patch is against wxWidgets 2.9.4.0, or more accurately, whatever wxWidgets ships with wxPython 2.9.4.0. I think they are the same but...

The only thing unsavory about this patch (full disclosure) is that really, in include/wx/osx/core/private.h, the line added in the patch

+ virtual int TabHitTest( const wxPoint & WXUNUSED(pt), long *flags ) {*flags=1; return -1;};

should really read

+ virtual int TabHitTest( const wxPoint & WXUNUSED(pt), long *flags ) {*flags=wxBK_HITTEST_NOWHERE; return -1;};

But that constant was not defined. I know that the right thing to do is probably to #include "bookctrl.h" in there, but I was a little leery to...I know how fragile the sequence of #includes in a large system can be, so I didn't want to just throw it in there. Maybe the pros can fix this up right. As it stands, 1 is the constant the wxBK_HITTEST_NOWHERE resolves to.

Of course, none of the above is relevant if TabHitTest is pure virtual...but when I tried that, other things broke because they obviously did not implement this virtual function. So to limit the scope of the patch, I provided the default implementation that returns a "miss".

comment:8 Changed 22 months ago by dhyams

  • Patch set

comment:9 Changed 22 months ago by dhyams

A word on the "unsavory" part of the patch that should be said...the refactoring suggested by VZ would remove that unsavory code anyway. The reason that it has to be there is that really TabHitTest() as well as a couple of other functions that deal with notebooks shouldn't really be in the class they are in.

Even the default implementation of TabHitTest() should never be called, so the fact that it hardcodes a constant isn't really an issue.

comment:10 Changed 22 months ago by dhyams

  • Summary changed from wxNotebook::HitTest() not implemented in wxOSX/Cocoa to [patch] wxNotebook::HitTest() not implemented in wxOSX/Cocoa

comment:11 Changed 22 months ago by dhyams

On this one, is there anything else that I need to do, in order to satisfy everyone that this patch is OK?

comment:12 Changed 22 months ago by csomor

apart from the detail that one should use the wxFromNSPoint method for converting, I don't see problems yet, but I will have to test

comment:13 Changed 21 months ago by SC

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

(In [73533]) applying patch, closes #13045

Note: See TracTickets for help on using tickets.