Skip to content

Simplify implementation of toolbar items#2117

Merged
barijaona merged 3 commits intoViennaRSS:masterfrom
Eitot:feature/toolbaritem-validation
Mar 29, 2026
Merged

Simplify implementation of toolbar items#2117
barijaona merged 3 commits intoViennaRSS:masterfrom
Eitot:feature/toolbaritem-validation

Conversation

@Eitot
Copy link
Copy Markdown
Contributor

@Eitot Eitot commented Mar 21, 2026

Note: This PR should be rebased on #2116, since macOS 10.15 is a requirement for this PR.

Since macOS 10.15 it is simpler to create button toolbar items by using the isBordered property and create pop-up button toolbar items with NSMenuToolbarItem. #2025 already implemented NSMenuToolbarItem, whereas this PR now implements the isBordered property.

With the isBordered property enabled, it is not necessary to set a custom view on the toolbar item to get the button appearance. In fact, NSToolbarItem doesn't use the view property when isBordered is enabled, so it cannot be referenced. When an action is sent from such a toolbar item, the sender isn't the button, but the toolbar item itself. Another downside I found is that macOS 10.15 needs a workaround for Interface Builder and some visual tweaks to make sure the buttons have the right size, but this was easily remedied.

I also took the opportunity to check whether the toolbar items are implemented correctly, since Vienna has extra code to make the overflow menu work (the menu that appears when the toolbar cannot show all toolbar items). By inspecting the default configuration of NSToolbarItem, reading the (outdated) documentation and looking at projects like GNUstep and Cocotron, I learned some new things about how the toolbar items work.

In text-only mode and the overflow menu, the toolbar displays the toolbar items' menuFormRepresentation. I found that these have to be overridden for toolbar items with custom views, but not for toolbar items with the isBordered property enabled or NSMenuToolbarItem objects. NSToolbarItem configures the default menuFormRepresentation with a private method on NSToolbarItem as the action and the toolbar item itself as the target. This private method forwards the toolbar item's action to the toolbar item's target (or first responder). NSToolbarItem also implements NSMenuItemValidation and validates the menu item itself. This means that a default toolbar item doesn't need any additional setup for the menuFormRepresentation. However, for NSToolbarItem objects with custom views, a custom menuFormRepresentation is still needed. In my testing I found that the the default NSToolbarItem behaviour can be mimicked by creating a private method on the toolbar item and setting it as the action of the menu item. NSToolbarItem will then validate the menu item as well. When the menu item is clicked (e.g. in text-only mode or the overflow menu), the private method can simply send the toolbar item's action. All of this makes the code much simpler.

I tested this on macOS 10.15, 11 and 26. I tested icon-only and text-only modes as well as the overflow menu. I also checked whether validation works as expected and made sure that #1578 doesn't reappear.

Also fixes #2115

@Eitot Eitot force-pushed the feature/toolbaritem-validation branch 2 times, most recently from fd12b17 to b9b5deb Compare March 24, 2026 18:18
@Eitot Eitot marked this pull request as ready for review March 24, 2026 18:21
@Eitot Eitot force-pushed the feature/toolbaritem-validation branch 2 times, most recently from 85a2e30 to 883f065 Compare March 25, 2026 18:57
@barijaona
Copy link
Copy Markdown
Member

When one clicks the "Share" icon, the menu appears at an unexpected position: at the leftmost part of the toolbar.
Capture d’écran 2026-03-26 à 13 01 08

Copy link
Copy Markdown
Member

@barijaona barijaona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sharing service toobar item behaves unexpectedly: the corresponding menu does not appear under the toolbar item.

@Eitot Eitot marked this pull request as draft March 26, 2026 22:13
@Eitot
Copy link
Copy Markdown
Contributor Author

Eitot commented Mar 26, 2026

Thanks for catching that, I completely overlooked this.

The switch to isBordered means that I cannot attach the share sheet to a view, which makes it impossible to pin the anchor point of the share sheet to the correct location, as macOS doesn't provide any API to get the coordinates of toolbar items.

I have checked out NSSharingServicePickerToolbarItem, but it is awkward to use since it doesn't use the standard target-action / toolbar validation pattern (instead requiring a custom delegate for both) and it seems to not work with text-only/overflow menu at all. There is also no documentation or further information on the web to give any clues on how to go about it. I am inclined to keep the custom NSButton toolbar item for this, but I'm afraid I cannot fix the share sheet location when the user chooses the text-only mode or accesses to share button from the overflow menu.

Edit: All other toolbar items work correctly though, I haven't noticed any different behaviours.

@Eitot Eitot force-pushed the feature/toolbaritem-validation branch from 883f065 to 2ce102c Compare March 26, 2026 22:36
@Eitot Eitot marked this pull request as ready for review March 26, 2026 23:56
@Eitot
Copy link
Copy Markdown
Contributor Author

Eitot commented Mar 26, 2026

@barijaona I made the changes and tested them on macOS 10.15, 11 and 26.

@barijaona
Copy link
Copy Markdown
Member

Unfortunately, the problem is still present if the toolbar is in text only mode.

@Eitot
Copy link
Copy Markdown
Contributor Author

Eitot commented Mar 28, 2026

Yes, as I've noted above, but that is already the case in the release version as well.

@Eitot Eitot force-pushed the feature/toolbaritem-validation branch from 083165f to 836b8fe Compare March 28, 2026 17:32
@Eitot
Copy link
Copy Markdown
Contributor Author

Eitot commented Mar 28, 2026

@barijaona I pushed a commit that should improve the situation for text-only mode. Still not ideal, but it's probably as good as it can get.

@Eitot Eitot marked this pull request as draft March 28, 2026 18:04
@Eitot Eitot marked this pull request as ready for review March 28, 2026 18:05
Eitot added 3 commits March 28, 2026 23:58
- Uses standard toolbar items with the isBordered property enabled. This
  makes it unnecessary to create custom views and menuFormRepresentation
  objects. Also adds a workaround and visual tweaks for macOS 10.15.
- Changes the implementation of the menuFormRepresentation object in the
  ButtonToolbarItem subclass to mimic the behaviour of standard toolbar
  items. This also removes the now unnecessary NSToolbar subclass.
- Merges PlugInToolbarItem and SharingServiceToolbarItem into a subclass
  that has a generic representedObject property, similar to menu items,
  and removes the PlugInToolbarItemButton subclass.
@Eitot Eitot force-pushed the feature/toolbaritem-validation branch from 836b8fe to 000c680 Compare March 28, 2026 23:00
@barijaona barijaona merged commit bee2eb9 into ViennaRSS:master Mar 29, 2026
2 checks passed
@Eitot Eitot deleted the feature/toolbaritem-validation branch March 29, 2026 05:09
@Eitot Eitot added the changes localisations This pull request adds, changes or removes localisation keys. label Mar 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes localisations This pull request adds, changes or removes localisation keys.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refresh icon not centered in Sonoma

2 participants