Opened 5 years ago

Closed 15 months ago

Last modified 15 months ago

#10661 closed defect (fixed)

Provide short event type contants names consistent with event table macros

Reported by: vadz Owned by:
Priority: normal Milestone: 3.0.0
Component: GUI-all Version:
Keywords: wxEventType API Cc:
Blocked By: Blocking:
Patch: yes

Description

Currently you may use EVT_MENU to connect a menu event handler in an event table but have to write wxEVT_COMMAND_MENU_SELECTED to do the same with Bind(). This is bad as it needlessly penalizes the users of the latter. A simple solution is to rename the event type constant to wxEVT_MENU too, keeping wxEVT_COMMAND_MENU_SELECTED only for compatibility.

Notice that some event table macros correspond to multiple events, e.g. EVT_MOUSE so there still won't be quite a 1-to-1 mapping between event table macros and event types. We probably can't do anything about this.

Attachments (1)

macro.patch download (706 bytes) - added by troelsk 15 months ago.
Reinstate wxDECLARE_DIRCTRL_EVT()

Download all attachments as: .zip

Change History (17)

comment:1 follow-up: Changed 16 months ago by catalin

For this I suppose the current constants should still be present as synonyms of the new ones. But should they be guarded by WXWIN_COMPATIBILITY_2_8 ?

For pairs of constants such as
EVT__AUINOTEBOOK_TAB_MIDDLE_DOWN
wxEVT_COMMAND_AUINOTEBOOK_TAB_MIDDLE_DOWN
what should be removed from the latter, "COMMAND" or "COMMAND_"? See the double underscore in the first name.

For
EVT_CHOICE / wxEVT_COMMAND_CHOICE_SELECTED
EVT_LISTBOX_DCLICK / wxEVT_COMMAND_LISTBOX_DOUBLECLICKED
The wxEVT_ name should always be constructed from "wx" and the shorter EVT_ name ?

comment:2 in reply to: ↑ 1 Changed 16 months ago by vadz

Replying to catalin:

For this I suppose the current constants should still be present as synonyms of the new ones. But should they be guarded by WXWIN_COMPATIBILITY_2_8 ?

No, I don't think so. There is just too much of the existing code using the current names for them to ever be really removed. And it doesn't cost anything to keep them neither.

For pairs of constants such as
EVT__AUINOTEBOOK_TAB_MIDDLE_DOWN
wxEVT_COMMAND_AUINOTEBOOK_TAB_MIDDLE_DOWN
what should be removed from the latter, "COMMAND" or "COMMAND_"? See the double underscore in the first name.

Definitely COMMAND_, we clearly don't want to have double underscores.

For
EVT_CHOICE / wxEVT_COMMAND_CHOICE_SELECTED
EVT_LISTBOX_DCLICK / wxEVT_COMMAND_LISTBOX_DOUBLECLICKED
The wxEVT_ name should always be constructed from "wx" and the shorter EVT_ name ?

Yes. The original motivation for this ticket was my desire to be able to write just wxEVT_BUTTON instead of wxEVT_COMMAND_BUTTON_CLICKED. Brief is good.

TIA for looking into this!

comment:3 follow-up: Changed 16 months ago by catalin

More questions:
1) Should the new constants replace the old ones in the code and the old ones be defined separately? Or just declare the new constants as the old ones?
2) What should be in the docs? Both old and new constants? Separated somehow? Or only the new ones?

comment:4 in reply to: ↑ 3 Changed 16 months ago by vadz

Replying to catalin:

More questions:
1) Should the new constants replace the old ones in the code and the old ones be defined separately? Or just declare the new constants as the old ones?

Whatever is more convenient, we just need to be consistent about it. I slightly prefer defining the old ones as the new ones to clearly show which are the preferred ones.

2) What should be in the docs? Both old and new constants? Separated somehow? Or only the new ones?

Definitely only the new ones, we want to point people to the preferred way.

comment:5 Changed 16 months ago by catalin

Should the aliases use wxDECLARE_EXPORTED_EVENT_ALIAS and wxDEFINE_EVENT_ALIAS or is enough to #define the old names as the new ones?

comment:6 Changed 16 months ago by vadz

After a quick glance at the sources, I don't see any reason to use the ALIAS macros there, AFAICS simple #defines should be good enough.

comment:7 Changed 16 months ago by catalin

There are some "generic command events", like EVT_COMMAND_LEFT_CLICK / wxEVT_COMMAND_LEFT_CLICK. I'll leave those unchanged.

comment:8 Changed 15 months ago by catalin

I found a difference between the sources and the docs regarding wxEVT_COMMAND_MENU_RANGE that I don't know how to resolve:

The sources:

#define EVT_MENU(winid, func) wx__DECLARE_EVT1(wxEVT_COMMAND_MENU_SELECTED, winid, wxCommandEventHandler(func))
#define EVT_MENU_RANGE(id1, id2, func) wx__DECLARE_EVT2(wxEVT_COMMAND_MENU_SELECTED, id1, id2, wxCommandEventHandler(func))
// here both COMMAND macros would change to wxEVT_MENU

The docs:

    @event{EVT_MENU_RANGE(id1, id2, func)}
        Process a @c wxEVT_COMMAND_MENU_RANGE command, which is generated by a range of menu items.

..but there is no wxEVT_COMMAND_MENU_RANGE in the sources.

Is it a bug in the sources or should the latter be removed from the docs?

comment:9 Changed 15 months ago by vadz

The docs are wrong, EVT_MENU_RANGE() processes the same event as EVT_MENU, i.e. wxEVT_COMMAND_MENU_SELECTED -- which should be renamed to wxEVT_MENU.

comment:10 Changed 15 months ago by catalin

  • Patch set

Ok.
I've added a patch and a review request: http://review.bakefile.org/r/481/

comment:11 Changed 15 months ago by VZ

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

(In [73850]) Provide shorter synonyms for wxEVT_XXX constants.

Use the same short names as are used by the event table macros for the event
type constants themselves. This makes them much more comfortable to use, e.g.
Bind(wxEVT_BUTTON) compared to Bind(wxEVT_COMMAND_BUTTON_CLICKED).

The old long names are still kept for backwards compatibility and shouldn't be
removed as it doesn't really cost anything to continue providing them, but all
new event types should only use the short versions.

Closes #10661.

comment:12 follow-up: Changed 15 months ago by troelsk

Added patch: Was it intentional to remove the wxDECLARE_DIRCTRL_EVT macro? The change broke my build as I'm using this macro in user space.

Changed 15 months ago by troelsk

Reinstate wxDECLARE_DIRCTRL_EVT()

comment:13 Changed 15 months ago by vadz

I don't know if it was intentional but this macro is definitely not public, so I'd advise you to switch to using wxDECLARE_EVENT_TABLE_ENTRY() in your code. FWIW this macro is not documented neither but it should be. And wx__DECLARE_EVTn() ones as well, probably...

comment:14 Changed 15 months ago by troelsk

Okay. Thanks

comment:15 in reply to: ↑ 12 Changed 15 months ago by catalin

Replying to troelsk:

Added patch: Was it intentional to remove the wx__DECLARE_DIRCTRL_EVT macro?

It was intentional because it really looked like a helper shortcut and was used for only one event, so what is now done in 1 line was being done in 3 or 4 lines before that change.

comment:16 Changed 15 months ago by VZ

(In [74032]) Propagate wxEVT_COMMAND_TEXT_UPDATED renaming to the real stc.cpp.

wxEVT_COMMAND_TEXT_UPDATED was renamed to wxEVT_TEXT in the generated stc.cpp
but not in stc.cpp.in it was generated from. Do it there too to prevent the
correct version in stc.cpp from being overwritten during the next regeneration.

See #10661.

Note: See TracTickets for help on using tickets.