macosui / macos_ui

Flutter widgets and themes implementing the current macOS design language.
https://macosui.github.io/macos_ui/#/
MIT License
1.86k stars 182 forks source link

Menu #89

Closed GroovinChip closed 3 years ago

GroovinChip commented 3 years ago
Screen Shot 2021-05-13 at 9 34 14 PM Screen Shot 2021-05-13 at 9 34 32 PM
GroovinChip commented 3 years ago

See https://gitlab.com/TheOneWithTheBraid/contextmenu/-/tree/main/lib/src for how to make this. Demo here https://theonewiththebraid.gitlab.io/contextmenu/#/

kiesman99 commented 3 years ago

Hey I'd like to try to implement this feature if help is still needed :)

However, duo to university stuff I can not promise keeping a deadline 😄

GroovinChip commented 3 years ago

Absolutely, please go for it!

kiesman99 commented 3 years ago

Is there a good exemplary application from which I should copy the macos design and feel?

I am kinda new to macos thus I don't really know where to find certain elements :)

Thank you!

lesliearkorful commented 3 years ago

@kiesman99 https://www.figma.com/file/M6K5L3GK0WJh6pnsASyVeE/macOS-Big-Sur-UI-Kit?node-id=26%3A5

GroovinChip commented 3 years ago

@kiesman99 I know its difficult for you to get a sense of your timeline because of school, but do you think you have a rough idea of when you think you can deliver this? Just so we know when to look out for it?

kiesman99 commented 3 years ago

@GroovinChip I'm playing a little with the api today to get a feel for it. I will respond after I've checked it out :)

GroovinChip commented 3 years ago

@GroovinChip I'm playing a little with the api today to get a feel for it. I will respond after I've checked it out :)

Sounds great, thank you!

kiesman99 commented 3 years ago

Hey @GroovinChip quick question:

The snippet you provided uses showModal which is a function coming from the animations package.

Is it okay to depend on this library or should I try to implement this feature using OverlayEntry or Remi Rousselet's flutter_portal (currently unmaintained)?

GroovinChip commented 3 years ago

I'd say let's try our own implementation, the less we depend on outside libraries the better.

GroovinChip commented 3 years ago

Looks like this is actually a duplicate of #53 but I'll keep this open since more movement is happening here

kiesman99 commented 3 years ago

Hey I've wanted to give a quick summary on what I've done today.

I've played around with the OverlayEntry and accomplished a rather basic Version of the contextmenu as you can see here:

context_menu_sample

Currently, I get an exception if remove the Overlay on exiting the region this has to be investigated.

Also, I don't know if we should proceed or wait with this issue as the context menu cannot be drawn outside of the flutter application with this approach.

Screenshot 2021-06-02 at 00 16 32

Whereas in Macos the context menu should display above the flutter application:

Screenshot 2021-06-02 at 00 23 56


I'd imagine such an implementation would require looking into the Flutter Macos Embedder. Doing a (very) quick research i could not find a method to implement this behavior, but as canonical is choosing flutter as the main framework to write desktop applications maybe they'll implement such a feature in the future.

kiesman99 commented 3 years ago

Regarding the 'deadline': I will try to implement this feature in the upcoming two weeks I think.

GroovinChip commented 3 years ago

Wow @kiesman99 this is amazing work, thank you! Maybe we can ask around the Flutter community for some ideas. I'd say you should keep going, this is really great!

GroovinChip commented 3 years ago

I tweeted this issue out here

kiesman99 commented 3 years ago

Thanks @GroovinChip!

I've created a WIP PR #125.

I am very excited if we could accomplish to place the context menu outside the flutter application. I'll keep an eye on that tweet :)

bdlukaa commented 3 years ago

I am very excited if we could accomplish to place the context menu outside the flutter application.

See https://github.com/flutter/flutter/issues/30701 for more info on this

bdlukaa commented 3 years ago

@kiesman99 While you can't render anything outside of the window (for security reasons), you can add something like a "padding" to make it fit the window.

When implementing tooltip, I found a method on the framework that does exactly that. It tries to position the child on the required offset, but if not possible, it handles that for you.

https://github.com/GroovinChip/macos_ui/blob/dev/lib/src/labels/tooltip.dart#L578

positionDependentBox can be found on flutter/rendering

kiesman99 commented 3 years ago

@bdlukaa i will look into this :)

My plan is to move the context menu to the left side if the user clicks somewhere near the right border but that wouldn't solve the issue of too small windows.

I think this is especially crucial, because context menus are limited in height and therefore in the number of elements. Also this is a red light to distinguish a flutter Mac_OS app from a native MacOS application.

bdlukaa commented 3 years ago

I think this is especially crucial, because context menus are limited in height and therefore in the number of elements.

We can do a few things to solve this:

kiesman99 commented 3 years ago

I think for now I'll go with the ScrollView or similar to solve the shrink issue.

Yesterday I've implemented a basic version of a ChangeNotifier that should keep track of all open ContextMenus.


How do we want to design the api? Do you want to provide a Widget in which the ContextMenu can be invoked

build(context) {
  return ContextMenuArea(
    menu: ContextMenu(
      children: [
        ContextMenuTile(),
        ContextMenuTile(),
        ContextMenuDivider(),
        ContextMenuTile(
          subMenu: ContextMenu(
            children: [
              // some more items for the submenu
            ]
          )
        )
      ]
    )
    child: Child() // inside of this child the user is able to invoke the context menu
  );
}

The ContextMenuArea handles the right clicks on the Child().

Or do we want to provide an imperative approach similar to showDialog?

void _contextMenu(TapDownDetails details) {
  // actual call to open context menu
  showContextMenu(
    position: details.globalPosition,
    builder: (context) {
      // build context menu in build function
      return ContextMenu(
        children: [
          ContextMenuTile(),
          ContextMenuTile(),
          ContextMenuDivider(),
          ContextMenuTile(
            subMenu: ContextMenu(
              children: [
                // some more items for the submenu
              ]
            )
          )
        ]
      );
    }
  );
}

build(context) {
  return GestureDetector(
    onSecondaryTapDown: (details) => _contextMenu(details)
    child: Child()
  );
}

Here is an issue from Remi in which he discusses the imperative approach of Overlay. This might help to form an opinion on this topic.

Thanks for your help!

GroovinChip commented 3 years ago

I personally am in favor of the second approach, the show function.

bdlukaa commented 3 years ago

Or do we want to provide an imperative approach similar to showDialog?

I don't think the showDialog approach will work with this. The current design seems good for me

kiesman99 commented 3 years ago

@bdlukaa I am not certain, because I could not yet test this approach, but I think it would work as Overlay is already imperative (see) and the Overlay is provided in the Navigator. Thus i can use the Overlay provided by the Navigator to inject context menus.

But I think this is a design decision. I think solving this with Widgets is a bit nicer, because its simpler to use for the end user of the library, but providing the show method gives the user more control.

However, if the show function can be implemented I could just provide a ContextMenuArea which abstracts the usage of the show method. This however could be confusing, because there is no "right" way of providing a ContextMenu (The user could be confused which approach he/she should use).

kiesman99 commented 3 years ago

The show method could be a bit more intuitive, because all flutter material overlays use this approach.

GroovinChip commented 3 years ago

The show method could be a bit more intuitive, because all flutter material overlays use this approach.

I vote for the show method

bdlukaa commented 3 years ago

because all flutter material overlays use this approach.

Tooltip doesn't.


The thing is that implementing the following would be harder with the show method:

Sorry for bad quality

GroovinChip commented 3 years ago

This may be of some interest: https://matejknopp.com/post/introducing-nativeshell/?s=09

kiesman99 commented 3 years ago

@GroovinChip That's awesome! Unfortunately I think requiring NativeShell for Macos would be cumbersome for many people and hinder them using this awesome library.

As more and more big companies (canonical for example) are adopting flutter I could imagine this kind of feature could be implemented by them native into flutter.

As cool as this embedder extension is I think we should keep the current native flutter solution and adopt a new official approach once it's available.

kiesman99 commented 3 years ago

@bdlukaa you're right. Tooltip abstracts the usage of Overlay. Isn't this a special case as tooltips can be applied to a multitude of widgets, thus having this kind of overlay as an attribute is very convenient.

As for the gif you showed: What would stop you from using the show method from implementing such a Toolbar (the toolbar here is different from the one discussed before)? You'd have to pass the position which you'll get on hovering over the menus and then the submenu will get rendered on this position.

Could you elaborate?

bdlukaa commented 3 years ago

What would stop you from using the show method from implementing such a Toolbar?

Nothing, but there should be a widget that abstracts and implements that.

kiesman99 commented 3 years ago

What would stop you from using the show method from implementing such a Toolbar?

Nothing, but there should be a widget that abstracts and implements that.

A more specific element such as Toolbars or these ActionButtons should abstract the usage of showContextMenu and we can do that.

A general "I want to right click something and get a context menu" would be easier using a show method. Further abstraction is still possible for specific use cases. The question here is "which elements should get abstracted" IMO.

Also in this PR I'd solely focus on the right click menu but I'll keep in mind that we may want to use the implementation in different spots.


It's kinda funny that I started with the mindset A widget would be better but this discussion is shifting me to the show-method approach 😅

GroovinChip commented 3 years ago

Also in this PR I'd solely focus on the right click menu but I'll keep in mind that we may want to use the implementation in different spots.

I think this is a good idea @kiesman99

kiesman99 commented 3 years ago

Hey there :)

Quick update! Unfortunately I couldn't do much, but there is still some progress.

I've implemented a rudimentary showContextMenu which can be used to initiate a context menu. This function however is not used to show the submenus. Instead, there is a ContextMenuNotifier which deals with the logic internally an. Here a quick sample:

progress

I am not quite satisfied with the current API which has to be used to create the context menu. But as I said this is a rudimentary approach as I am still researching on how others are implementing context menus.

Things I want to approach as next steps:

GroovinChip commented 3 years ago

Excellent! And how does the menu behave when opened near a window boundary?

maheshj01 commented 3 years ago

I believe the position of the context menu should be altered when opening near the window boundaries. e.g opening a context menu on the rightmost bottom should be moved left (with a minimum width offset of context Menu) and up with (a minimum height offset of context Menu). Similarly considering this in all 4 directions. I believe this will solve for not having to deal with multiple window support, As this is not yet supported in flutter.

GroovinChip commented 3 years ago

@kiesman99 Do you have any further updates to share?

kiesman99 commented 3 years ago

Hey @GroovinChip, I am very sorry for not responding.

Unfortunately, a few of my exams were brought forward. Therefore, I have shifted the focus a little.

I have made a few minor changes and would start there again when the exams are over. If you prefer, I can also do a first working version, which I'll tweak a bit afterwards.

GroovinChip commented 3 years ago

Hey @GroovinChip, I am very sorry for not responding.

Unfortunately, a few of my exams were brought forward. Therefore, I have shifted the focus a little.

I have made a few minor changes and would start there again when the exams are over. If you prefer, I can also do a first working version, which I'll tweak a bit afterwards.

No worries @kiesman99 thanks for the update. We can wait till your exams are over.

GroovinChip commented 3 years ago

@kiesman99 any news on this? Are your exams over?

GroovinChip commented 3 years ago

@kiesman99 any movement on this?

kiesman99 commented 3 years ago

Hey @GroovinChip, I am very sorry for not responding for a while. Duo to private matter and the exams I hadn't had the time for social media. This semester was way more stressful than the other ones and I underestimated it. Now my exams are done and I will focus again on the menu implementation.

GroovinChip commented 3 years ago

Excellent, I'm glad to hear it!

GroovinChip commented 3 years ago

CC @lesnitsky

kiesman99 commented 3 years ago

Hey @GroovinChip I think you can start to review #125.

In the next days I am going to implement adapting the system design, which is the last missing part.

This version does not include submenus, because I've done a whole re-write of my first implementation, taking some inspiration on the PopupMenuButton from flutter.

Submenus will be included in a separate PR after these "simple" context menus are reviewed and merged (if that's okay to you)

GroovinChip commented 3 years ago

Sure, that's fine. Thanks for your hard work! I will review in the near future, likely either tomorrow or next week.

Edit: cc @lesnitsky

lesnitsky commented 3 years ago

hey all awesome work here! I've published native_context_menu recently

I was looking into context menu implementations in flutter repo and chose a native approach for a couple of reasons, the main was native styling. As good as flutter is in rendering part it's still hard to make things look exactly like native. Imo context menu is one of the most used native controls, so users are very used to how it looks and behaves. custom implementations are always kind-of confusing. + macos has some nice ux touches (e.g. submenu waits a bit before hiding when cursor exits the parent menu item, so diagonal gesture has less invalid resulting items selected)

we can collaborate to integrate native_context_menu "natively" to macos_ui, lmk wdyt

GroovinChip commented 3 years ago

@kiesman99 what do you think about collaborating with @lesnitsky about folding his native implementation into macos_ui? I think it could be a good approach. I know you've worked super hard on this, and I don't want your work to go to waste, so I think a collaboration between the two of you is a good middle ground. We'd get native behavior and you'd still get to work on this issue.

kiesman99 commented 3 years ago

@GroovinChip, @lesnitsky I see the benefits of using a native approach and it would be totally fine, if we'd decide to take @lesnitsky's implementation.

I like that the native approach feels on spot, because it is native and we are able to have a context menu that has an offset to the actual flutter window. One drawback that I can see however is custom items.

For example in the finder app we can choose coloured tags:

Screenshot 2021-09-25 at 22 31 21

If a user wants to have such a custom item he would need to implement it in swift right @lesnitsky?

kiesman99 commented 3 years ago

With a flutter non-native implementation the context menu will always be bound by the flutter window size, but it's possible to implement custom items.

There is the ContextMenuEntry<T> class which can be used to implement custom tiles with custom behaviour:

class CustomContextMenuItem<T> extends ContextMenuEntry<T> {
  const CustomContextMenuItem({Key? key}) : super(key: key);

  @override
  _CustomContextMenuItemState<T> createState() => _CustomContextMenuItemState<T>();
}

/// [ContextMenuEntry.handleTap] is not used in this state, because we want to have 3 buttons which
/// each should return a seperate value
class _CustomContextMenuItemState<T> extends ContextMenuEntryState<CustomContextMenuItem, T> {
  @override
  Widget build(BuildContext context) {
    return Container(
      child: Row(
        children: [
          TextButton(onPressed: () {
            Navigator.pop<int>(context, 3);
          }, child: Text('3')),
          TextButton(onPressed: () {
            Navigator.pop<int>(context, 4);
          }, child: Text('4')),
          TextButton(onPressed: () {
            Navigator.pop<int>(context, 5);
          }, child: Text('5'))
        ],
      )
    );
  }
}

which will result in:

Screenshot 2021-09-25 at 22 36 59