Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New toolbar button control with dropdown context menu (for Wpf, Gtk, Mac; #2182) #2243

Closed
wants to merge 10 commits into from
Closed

New toolbar button control with dropdown context menu (for Wpf, Gtk, Mac; #2182) #2243

wants to merge 10 commits into from

Conversation

Serg-Norseman
Copy link
Contributor

Implemented an additional toolbar button control that displays a dropdown menu aligned to the bottom left corner of the button by clicking with the left mouse button. The basic functionality of the button is identically copied from ButtonToolItem.

Can replace cases where a missing ComboBoxToolItem is needed.

More convenient than the ordinary implementation of ButtonToolItem -> ClickHandler -> ContextMenu.Show(...), because displays an aligned menu rather than at the cursor position.

It is also convenient in that it allows you to set the menu for button and its items in xeto, instead of code.

@cwensley
Copy link
Member

cwensley commented Aug 6, 2022

Hey @Serg-Norseman, thanks so much for this PR! Do you have time to look into the build errors so I can merge this? I think it's mainly due to not dealing with the MonoMac vs Xamarin.Mac vs MacOS namespaces. Those have now moved into global usings so you shouldn't need any in the source files anymore anyway. There's also a GTK3 compile error, which you can wrap around #if GTKCORE if there's no GtkSharp3 equivalent.

@Serg-Norseman
Copy link
Contributor Author

Of course! If you find this PR useful - I will study and try to fix all the problems! I don’t always understand the essence of the errors there, but I’ll work on them. I’ll start on Monday-Tuesday :)

@cwensley
Copy link
Member

Hm, looks like the handlers aren't being added in each of the corresponding Platform.cs files. Could you add those? Otherwise it looks great.

@Serg-Norseman
Copy link
Contributor Author

I didn't want to make changes to these files without approval :)

@cwensley
Copy link
Member

Ok, got some time to look at the platform specifics. Thanks for your patience. This looks good though ideally we could use Gtk.MenuToolButton for Gtk, and NSMenuToolbarItem on macOS. Let me know if you need help making these changes, which I'm more than happy to do on my end.

@allsorts46
Copy link
Contributor

Sorry to jump in here - I saw the discussion and this is something I'd like to see added. I actually already implemented this for Winforms, WPF, GTK and Android and was going to propose a PR for if/when the other Android changes could be accepted.

This PR looks good too though, happy to drop my changes and use this if it gets merged. This one takes a slightly different approach, with DropDownToolItem using the ContextMenu property to attach a drop-down, whereas I just added an Items collection directly to mine instead. The difference is just preference I guess.

I just wanted to comment regarding what @cwensley said about using for MenuToolButton for GTK. I tried this as well at first but it had a couple of problems, at least under GTK3:

  • The main button and the drop down button are separate and I could not find any way to combine them. Perhaps you could wire up the main button click to trigger the drop down but as far as GTK is concerned they're two separate things. Like ToolStripSplitButton vs ToolStripDropDownButton in Winforms.
  • The buttons are visually separate too, and look very ugly

GTK4 appears to include a control that does combine a single button with a dropdown, but I couldn't find any such thing for GTK3.

My solution was to use ToolButton as @Serg-Norseman has done, but use a Unicode arrow to visually indicate it's a drop-down. Sounds a bit hacky but it was the best-looking compromise I could find:

MenuToolButton

ToolButton

(sorry for the poor GIF quality)

Adding the character is handled internally:

public override void CreateControl(ToolBarHandler handler, int index)
	{
	Gtk.Toolbar tb = handler.Control;
	Control = new Gtk.ToolButton(GtkImage, Text + "  ▾");
	/* Other properties */
	}

It would probably be a good idea to make the ToolItemHandler.Text virtual as well so it can be overridden in DropDownToolItemHandler to ensure the GTK button text stays in sync if the Eto button text is changed.

Hope this is useful in some way. As mentioned, I have an Android implementation ready as well, should this get merged.

@Serg-Norseman
Copy link
Contributor Author

I won't be able to make changes for macOS because I don't have access to it. The development of such an implementation was done by my colleague on the project that uses Eto.

Adding a down arrow would be nice to make optional - because in some cases it is very useful and necessary, I did it myself. But sometimes it's not necessary. In general, it seems to me that it is better to leave it to the discretion of the one who uses the component.

It seems to me that it is more preferable to use the ContextMenu, because allows you to use the various features of the menu items, such as images for example. Or hotkeys, or nested submenus and separator lines - I really need this.

I won't be able to test @cwensley's suggestion until next week, so I can't comment on that.

It would probably be a good idea to make the ToolItemHandler.Text virtual as well so it can be overridden in DropDownToolItemHandler to ensure the GTK button text stays in sync if the Eto button text is changed.

I support it!

@cwensley
Copy link
Member

Thanks for the feedback @Serg-Norseman and @allsorts46.

It sounds like Gtk.MenuToolButton is more of a split button so yeah it won't be usable for this. We could certainly make the arrow optional by adding a property to the handler which one could change via a style.

No worries about Mac, I'll make any necessary changes there and do some testing to make sure it's suitable.

@allsorts46
Copy link
Contributor

@Serg-Norseman Regarding separate ContextMenu or not, I'm not sure there's a difference in customisation of menu items, as my implementation is still implemented as a MenuItemCollection, just the same as ContextMenu does. The advantage is just not having to manually create and attach your own extra ContextMenu instance (why would you want a DropDownToolItem without any dropdown items?). But I may be missing something regarding ContextMenu's handling in terms of keyboard shortcuts, etc. Not sure.

I agree it's nice to have the option of showing the drop down arrow or not, but I'd argue the case in favour of having it enabled by default, just so that by default the DropDownToolItem doesn't look exactly the same as a ButtonToolItem, and users have a hint that it's a menu.

@cwensley
Copy link
Member

cwensley commented Aug 25, 2022

Regarding separate ContextMenu or not, I'm not sure there's a difference in customisation of menu items, as my implementation is still implemented as a MenuItemCollection, just the same as ContextMenu does. The advantage is just not having to manually create and attach your own extra ContextMenu instance (why would you want a DropDownToolItem without any dropdown items?). But I may be missing something regarding ContextMenu's handling in terms of keyboard shortcuts, etc. Not sure.

I had thoughts around this too. No real need to have a ContextMenu instance here, and would simplify the creation of the DropDownToolItem. e.g.

new DropDownToolItem { Items = { new ButtonMenuItem(), new SeparatorMenuItem() } }

vs.

new DropDownToolItem { ContextMenu = new ContextMenu { Items = { new ButtonMenuItem(), new SeparatorMenuItem() } } }

xaml would be much nicer too:

<DropDownToolItem><ButtonMenuItem /><SeparatorMenuItem /></DropDownToolItem>

vs.

<DropDownToolItem>
  <DropDownToolItem.ContextMenu>
    <ContextMenu>
      <ButtonMenuItem />
      <SeparatorMenuItem />
    </ContextMenu>
  </DropDownToolItem.ContextMenu>
</DropDownToolItem>

I agree it's nice to have the option of showing the drop down arrow or not, but I'd argue the case in favour of having it enabled by default

I agree that making it enabled by default makes sense.

@Serg-Norseman
Copy link
Contributor Author

You are both very convincing :) I agree with all the arguments :)

@Serg-Norseman
Copy link
Contributor Author

Maybe promote @allsorts46's implementation then? Because he has already done everything as required and for all platforms. I do not insist on my contribution - the main thing for me is that the functionality is available.

@allsorts46
Copy link
Contributor

I'm happy to share my version as well, to compare and/or combine with this. I just need to tidy up a little to make sure I can do a clean commit of only this feature and nothing else. Will do it this evening.

@allsorts46
Copy link
Contributor

I've submitted my version as #2299.

Originally got a build error because I used Gtk.Menu.PopupAtWidget() which is apparently only available in the Eto.Gtk platform but not Eto.Gtk3 or Eto.Gtk2. I used the popup location code from this PR in an #if for those platforms and continued to prefer PopupAtWidget() for the new platform.

Completely missing a Mac implementation unfortunately. Possibly the one from this PR can be adapted but I did not try as I wouldn't be able to test if it even builds or not.

Only thing I'm not happy about with the GTK implementation is that the button doesn't look 'pressed' whilst the dropdown is shown. Thinking about experimenting with using a ToggleToolButton internally instead and making it 'checked' for as long as the dropdown is visible. Though with GTK we have to worry about themes, and how they might choose to interpret and render 'checked' - might look weird on some themes. I'll also try to make the arrow optional.

@allsorts46
Copy link
Contributor

Tried the ToggleToolButton approach, works well and looks good with default theme, just unsure about custom themes.

It's now possible to disable the drop arrow via styles, like this:

Eto.Style.Add<Eto.GtkSharp.Forms.ToolBar.DropDownToolItemHandler>(null, h => h.ShowDropArrow = false);

@cwensley
Copy link
Member

I've now merged a fix that include the DropDownToolItem based on @allsorts46 implementation. Please give it a go and let me know if there's anything amiss. Thanks @allsorts46 and @Serg-Norseman for doing the legwork to get this implemented! It will be a very useful feature for Eto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants