Customize toolbar on OSX #23614

oneeyeman1 · 2023-06-05T23:38:37Z

This is an attempt to introduce the toolbar customization functionality into the library.

However, I have couple of questions:

Should this functionality be turned on based on the style/flag?
According to the Apple documentation it is recommended to store the new toolbar configuration in the user preferences, so that it can be read it from on the next restart. Should this be turned on by some flag?
I don't know if there should be any unit test for this, but I'd like to demonstrate it in the sample? What's the best way to do that? I'm thinking to turn off some buttons and show the customization dialog that will allow to drop the tool on the toolbar. Which one would you recommend?

Please review and give me your thoughts. Only OSX implementation is here - it is supported natively by OS.

MSW will follow and then GTK.

On OSX it is available with the right click and context menu selection.
On MSW it will be available thru the double click.
On GTK I'm thinking to follow OSX way as the GtkToolbar have a "context-menu" signal. And in GTK4 it probably can be done the same way on the GtkBox.

(This PR is not ready to be applied, unless you want it to get some testing on OSX first).

Thank you.

… to setup panel

Temporary fix the compilation error on MSW to satisfy the test build

Fix the parameter name typo

Fix GTK build (for now)

Fix error about assigning the variable to itself

Fix Universal build

Fixing Universal build

vadz · 2023-06-06T19:23:55Z

I have 2 comments about this:

Please test your PRs in your own fork of wxWidgets when they haven't even reached compilable state yet, it's annoying to have to wait until CI builds for this PR finish before being able to run any other ones. In the meanwhile at least switch this one to the "Draft" state until it's really finished.
Adding bool available parameter is not the right way to do whatever you want to do. Please don't do this, you should be able to add some wxToolBarTool::MarkAsAvailable() or whatever instead.

Fix GTK bujild

oneeyeman1 · 2023-06-06T20:35:53Z

@vadz,

I have 2 comments about this:

Please test your PRs in your own fork of wxWidgets when they haven't even reached compilable state yet, it's annoying to have to wait until CI builds for this PR finish before being able to run any other ones. In the meanwhile at least switch this one to the "Draft" state until it's really finished.

I don't have clang installed anywhere but on OSX, so I can't test it under this compiler.
But thank you for pointing out the "draft" switch. Now marked.

How do I unmark it from that? Should I just click on the "work-needed" link?

Adding bool available parameter is not the right way to do whatever you want to do. Please don't do this, you should be able to add some wxToolBarTool::MarkAsAvailable() or whatever instead.

OK, thx.
Will do that. Probably name it "MarkAvailable()".
Let me know if the name is not up to coding standard.

Thx as always.

oneeyeman1 · 2024-01-12T17:33:19Z

@vadz,
I think it is now ready.

It would be nice if Stefan also could take a look.

Thx.

vadz

We need to check if this can be made to work elsewhere. Providing at least a generic implementation would be a good way to ensure we can do it, as long as this is only supported under Mac I'm not sure if we can use such API.

vadz · 2024-01-29T01:39:08Z

include/wx/control.h

+    void MarkAvailable(bool available) { m_available = available; }
+    bool IsAvailable() const { return m_available; }


We shouldn't need to add functions that only make sense for toolbar tools to base wxControl class. You will have to change the code to use them in wxToolBatTool only.

vadz · 2024-01-29T01:39:39Z

include/wx/event.h

@@ -724,6 +724,7 @@ wxDECLARE_EXPORTED_EVENT(WXDLLIMPEXP_CORE, wxEVT_COMBOBOX, wxCommandEvent);
 wxDECLARE_EXPORTED_EVENT(WXDLLIMPEXP_CORE, wxEVT_TOOL_RCLICKED, wxCommandEvent);
 wxDECLARE_EXPORTED_EVENT(WXDLLIMPEXP_CORE, wxEVT_TOOL_DROPDOWN, wxCommandEvent);
 wxDECLARE_EXPORTED_EVENT(WXDLLIMPEXP_CORE, wxEVT_TOOL_ENTER, wxCommandEvent);
+wxDECLARE_EXPORTED_EVENT(WXDLLIMPEXP_CORE, wxEVT_TB_CUSTOMIZE, wxCommandEvent);


Just TB is really not clear enough without any context.

Suggested change

wxDECLARE_EXPORTED_EVENT(WXDLLIMPEXP_CORE, wxEVT_TB_CUSTOMIZE, wxCommandEvent);

wxDECLARE_EXPORTED_EVENT(WXDLLIMPEXP_CORE, wxEVT_TOOLBAR_CUSTOMIZE, wxCommandEvent);

vadz · 2024-01-29T01:40:53Z

include/wx/osx/toolbar.h

    virtual wxToolBarToolBase *CreateTool(wxControl *control,
                                          const wxString& label) override;

+    void *GetMacToolbar() { return m_macToolbar; };


The prevailing convention is to use OSX prefix, i.e.

Suggested change

void *GetMacToolbar() { return m_macToolbar; };

void *OSXGetToolbar() { return m_macToolbar; };

vadz · 2024-01-29T01:41:29Z

include/wx/osx/toolbar.h

@@ -89,9 +89,11 @@ class WXDLLIMPEXP_CORE wxToolBar: public wxToolBarBase
                                          wxObject *clientData = nullptr,
                                          const wxString& shortHelp = wxEmptyString,
                                          const wxString& longHelp = wxEmptyString) override;
+


Please try to avoid whitespace-only changes (even if you make them accidentally, nothing prevents you from looking at the diff and removing them afterwards).

vadz · 2024-01-29T01:41:58Z

include/wx/tbarbase.h

@@ -314,7 +319,7 @@ class WXDLLIMPEXP_CORE wxToolBarBase : public wxControl
                               const wxString& shortHelp = wxEmptyString,
                               wxItemKind kind = wxITEM_NORMAL)
    {
-        return AddTool(toolid, label, bitmap, wxBitmapBundle(), kind, shortHelp);
+        return AddTool(toolid, label, bitmap, wxBitmapBundle(), kind, shortHelp, wxEmptyString, nullptr);


vadz · 2024-01-29T01:42:46Z

interface/wx/toolbar.h

@@ -159,6 +159,19 @@ class wxToolBarToolBase : public wxObject

    void SetDropdownMenu(wxMenu *menu);
    wxMenu *GetDropdownMenu() const;
+
+    /**
+     Mark the tool to be not visible.


This is pretty misleading for a function called like this. You should say it may be used to mark the tool as being not currently visible if @c available is @false or something like that.

vadz · 2024-01-29T01:43:58Z

interface/wx/toolbar.h

+    bool IsAvailable() const;
+
+    /**
+        Calling this function with the parameter set to false will not add the tool to the toolbar


Suggested change

Calling this function with the parameter set to false will not add the tool to the toolbar

Calling this function with the parameter set to false prevents the tool from being added to the toolbar.

vadz · 2024-01-29T01:44:30Z

samples/toolbar/toolbar.cpp

@@ -470,6 +470,7 @@ void MyFrame::PopulateToolbar(wxToolBarBase* toolBar)
        combo->Append("combobox with extremely, extremely, extremely, extremely long label");
        combo->Append("in a");
        combo->Append("toolbar");
+        combo->MarkAvailable( false );


Please create another initially unavailable control instead of hiding this one.

vadz · 2024-01-29T01:45:12Z

src/osx/iphone/toolbar.mm

This file is not changed at all...

vadz · 2024-01-29T01:46:50Z

interface/wx/toolbar.h

+        Process a @c wxEVT_TB_CUSTOMIZE event. Under MSW and OSX
+        it will display native customization dialogs. On GTK it will
+        be generic.


This doesn't seem to be true, there is no implementation at all for MSW and just a stub for GTK. This is not necessarily a problem, but the documentation shouldn't like and you should check that the API you propose is at least in principle implementable under the other platforms.

oneeyeman1 · 2024-01-29T01:56:13Z

@vadz ,

We need to check if this can be made to work elsewhere. Providing at least a generic implementation would be a good way to ensure we can do it, as long as this is only supported under Mac I'm not sure if we can use such API.

On MSW it is possible to customize toolbar.

GTK does not support that and apparently the devs there are objecting to make it work.

And of course generic implementation we can do whatever we want,
We already have an API to do customization in the generic way..

oneeyeman1 · 2024-01-29T02:01:56Z

Also, I started with OSX, because I am not very familiar with OSX.

I will follow you review and apply where appropriate.

However, I'd like to ask you - if this will become mergeable will it be easier to do the MSW here or merge it and then change MSW?

Let me know.

Thx.

oneeyeman1 added 20 commits May 31, 2023 23:06

Implement toolbar customization on OSX - initial commit

567b632

Add parameter to differentiate between visible and available items

10c87b1

Implement delegate methods for the toobar customization panel

89a8a7f

Adding control to implementation

779f9c1

Fix compilation error

e0060ae

Realize does not add anything to the toolbar, so no need to add tools…

57f5df6

… to setup panel

Merge remote-tracking branch 'upstream/master' into CustomizeToolbar

a632e68

Fixing GTK implementation and starting implementing the signal

4fcc972

Fix MSW compilation

abfab1d

Fix trailing whitespaces

8be1d3a

Fix spelling

92728d8

Fix compilation under OSX

ce48129

Trying to fix iPhone build

35392ef

Fix documentation

029c0c1

Update toolbar.cpp

9dc456b

Temporary fix the compilation error on MSW to satisfy the test build

Update toolbar.cpp

24cb97c

Fix the parameter name typo

Update toolbar.cpp

f18e6b9

Fix GTK build (for now)

Update buttonbar.cpp

7543be5

Fix error about assigning the variable to itself

Update toolbar.h

73f1a88

Fix Universal build

Update toolbar.cpp

489a21d

Fixing Universal build

vadz added the work needed Too useful to close, but can't be applied in current state label Jun 6, 2023

Update toolbar.cpp

6e0f893

Fix GTK bujild

oneeyeman1 marked this pull request as draft June 6, 2023 20:30

oneeyeman1 added 5 commits June 8, 2023 23:01

Update the documentation and trying to implement the PR suggestion

37f9368

Merge remote-tracking branch 'upstream/master' into CustomizeToolbar

9883ce5

Implement PR suggestion

1a4de0b

Fix code checks (white spaces)

76c1d5d

Revert to the old code

f83da65

oneeyeman1 added 7 commits June 14, 2023 18:37

Revert changes for UNIVERSAL build

8980a62

Fix whitespaces

9d7a875

Reimplement the feature according to PR suggestions

250879e

Implement customization toolbar on OSX for the regular button

a49443d

Updating the documentation

57dd140

Fix the whitespaces

7606d55

Fix compilation error

fe732c7

oneeyeman1 marked this pull request as ready for review January 12, 2024 17:32

vadz reviewed Jan 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Customize toolbar on OSX #23614

Customize toolbar on OSX #23614

oneeyeman1 commented Jun 5, 2023

vadz commented Jun 6, 2023

oneeyeman1 commented Jun 6, 2023

oneeyeman1 commented Jan 12, 2024

vadz left a comment

vadz Jan 29, 2024

vadz Jan 29, 2024

vadz Jan 29, 2024

vadz Jan 29, 2024

vadz Jan 29, 2024

vadz Jan 29, 2024

vadz Jan 29, 2024

vadz Jan 29, 2024

vadz Jan 29, 2024

vadz Jan 29, 2024

oneeyeman1 commented Jan 29, 2024

oneeyeman1 commented Jan 29, 2024

		void MarkAvailable(bool available) { m_available = available; }
		bool IsAvailable() const { return m_available; }

	wxDECLARE_EXPORTED_EVENT(WXDLLIMPEXP_CORE, wxEVT_TB_CUSTOMIZE, wxCommandEvent);
	wxDECLARE_EXPORTED_EVENT(WXDLLIMPEXP_CORE, wxEVT_TOOLBAR_CUSTOMIZE, wxCommandEvent);

	void *GetMacToolbar() { return m_macToolbar; };
	void *OSXGetToolbar() { return m_macToolbar; };

	Calling this function with the parameter set to false will not add the tool to the toolbar
	Calling this function with the parameter set to false prevents the tool from being added to the toolbar.

Customize toolbar on OSX #23614

Are you sure you want to change the base?

Customize toolbar on OSX #23614

Conversation

oneeyeman1 commented Jun 5, 2023

vadz commented Jun 6, 2023

oneeyeman1 commented Jun 6, 2023

oneeyeman1 commented Jan 12, 2024

vadz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oneeyeman1 commented Jan 29, 2024

oneeyeman1 commented Jan 29, 2024