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

[wip] menu applet: Make the menu resizeable. #9771

Closed
wants to merge 6 commits into from

Conversation

mtwebster
Copy link
Member

@mtwebster mtwebster commented Dec 13, 2020

This adds a class in js/ui/applet.js that can be attached to an applet
to allow it to be resized using the mouse.

This is currently only implemented for the menu applet but can be expanded
in the future.

The layout for the applet was cleaned up to allow proper functioning of
the three scroll view actors.

Issues:

  • The extra allocation for the scrollbars don't go away even though the
    scrollbar itself is invisible.
  • Because of this maybe we want to pad the category labels with a few
    trailing spaces to counter the space lost from the new scrollbar.
  • In certain panel positions, resizing in one direction may result in
    changes to the position of the opposite edge due to how the panel calculates
    popup menus. This doesn't affect function, and in reality probably won't be
    an issue for many people.

Peek 2020-12-13 17-21

@mtwebster mtwebster force-pushed the dnd-menu-resizing branch 3 times, most recently from edb8c7d to ce017a1 Compare December 13, 2020 22:44
@clefebvre
Copy link
Member

Segfault while resizing it with the mouse: https://termbin.com/rqvy. It resized many times without crashing but eventually it crashed :)

After I resize I also have another problem. If the menu is too small for my categories I get a scrollbar for them.. that's ok... but scrolling there with the mouse wheel is clunky, it doesn't respond well and sometimes ignores the wheel motion. I get the feeling the hover system (to activate categories), or maybe the vector, gets in the way.

NikoKrause and others added 5 commits December 15, 2020 11:51
resizing of the menu is not ideal and needs to be improved in the long
run
This adds a class in js/ui/applet.js that can be attached to an applet
to allow it to be resized using the mouse.

This is currently only implemented for the menu applet but can be expanded
in the future.

The layout for the applet was cleaned up to allow proper functioning of
the three scroll view actors.

Minor issues:

The extra allocation for the scrollbars don't go away even though the
scrollbar itself is invisible. Because of this maybe we want to pad the
category labels with a few trailing spaces to counter the space lost from
the new scrollbar.

Also, in certain panel positions, resizing in one direction may result in
changes to the position of the opposite edge due to how the panel calculates
popup menus. This doesn't affect function, and in reality probably won't be
an issue for many people.
categories).

The vector box is now angle/distance calculations to determine whether
movement from a category to apps is within the range to maintain the
current category.
@mtwebster
Copy link
Member Author

Category scrolling fixed. I had to rewrite the vector stuff but it seems to be as effective.

I can't reproduce the crash, will keep an eye out.

@brownsr
Copy link
Member

brownsr commented Dec 15, 2020

I get a lot of these errors on trying to drag. Initial ones below:

Gdk-Message: 17:25:11.587: cinnamon-session: Fatal IO error 11 (Resource temporarily unavailable) on X server :0.

Gdk-Message: 17:25:11.588: csd-a11y-settings: Fatal IO error 11 (Resource temporarily unavailable) on X server :0.

Gdk-Message: 17:25:11.589: csd-media-keys: Fatal IO error 11 (Resource temporarily unavailable) on X server :0.

Gdk-Message: 17:25:11.593: csd-print-notifications: Fatal IO error 11 (Resource temporarily unavailable) on X server :0.

@mtwebster
Copy link
Member Author

Crazy I never saw anything like that. It's just using DND.makeDraggable.
What theme?

@brownsr
Copy link
Member

brownsr commented Dec 15, 2020

Not a standard one ! I'll try again with Mint-Y

@mtwebster
Copy link
Member Author

I tested with Mint-*, cinnamon, Adapta-Nokto.

@mtwebster
Copy link
Member Author

Ok I noticed an issue in CBlack with resizing, I'll have to see what they're doing. I'll sample more as well.

@brownsr
Copy link
Member

brownsr commented Dec 15, 2020

Those x errors disappeared after a reboot, so ignore those, probably just me. It all seems far better in mint-y. graphite-zero was the theme I was using. I'll check that out again.

Well that's interesting. I changed back to the graphite-zero cinnamon theme, and am getting the same hang on drag and those x errors. I guess something must be getting well out of range !

@mtwebster
Copy link
Member Author

Does it work ok if you only attempt to drag at the corner?

@brownsr
Copy link
Member

brownsr commented Dec 15, 2020

yes, that works fine.

@mtwebster
Copy link
Member Author

Ok it's to do with the margins between the popup and panel - .menu.top, bottom, left, right

I just need to take this into account I think. Graphite-Zero doesn't crash here, it just grows uncontrollably (like CBlack).

@lestcape
Copy link

This is as some one that implemented the same some years ago. So, i hope this comment will be usefull here, otherwise please just ignored it.

This is a super nice feature and I'm speaking of get rid of St.Poligon in favour of just use math. The improve in performance of this was huge when i implemented this some years ago in the configurable menu fork. Here need to be the same performance increase.

The resize option is also nice, but in my opinion right now is only useful for resize the height and adapt the menu to several screen sizes. I think, it can be more useful later if it will come together with the possibility of to have more that one columns for the menu items. This is because right now the menu can increase the width but without the real advantage of display more content to the user. But anyway yes, more columns also means a reduction in performance, so it's also something to be thinking.

The feature of _update_scroll_policy it's a nice addition, but yes the exceeds width need to be fixed. Because of that I ended up fully hidden the scroll of the favourite box because of it. Then I preferred to add the auto-scroll with the mouse over feature there, but without showing the bar, to be preserved the width always. Probably it's better just not show the bar and instead add one button at top to scroll up and one at bottom to scroll down. But, they are just ideas and I have to admit that to me both things look like imperfect.

This still doesn't completely handle themes using margins for the
outer actor, but it at least keeps them somewhat in check.

- Don't update the width and height unless they've
  been deliberately changed.
- Some hidpi fixes.
- Add a delay to storing the new values in settings - updating at
  the same rate as we're updating the size during a drag can cause
  a Cinnamon crash.
@fredcw
Copy link
Contributor

fredcw commented Dec 19, 2020

@mtwebster Although more rarely than before, it's still causing cinnamon to freeze/crash when resizing the menu. Happened 3 times.

Seems to coincide with garbage collection. This was repeated in the .xsession-errors one time:

(cinnamon:4072): Cjs-CRITICAL **: 06:45:06.776: Attempting to call back into JSAPI during the sweeping phase of GC. This is most likely caused by not destroying a Clutter actor or Gtk+ widget with ::destroy signals connected, but can also be caused by using the destroy(), dispose(), or remove() vfuncs. Because it would crash the application, it has been blocked and the JS callback not invoked.

(cinnamon:4072): Cjs-CRITICAL **: 06:45:06.776: The offending signal was allocate on CinnamonGenericContainer 0x55a4aa81b360.
== Stack trace for context 0x55a4aa7cd180 ==
#0   7fffa0bc1770 I   /usr/share/cinnamon/js/ui/popupMenu.js:2562 (3e0d7200cb50 @ 28)
#1   7fffa0bc17c0 I   self-hosted:844 (d1ddd8c2970 @ 492)
#2   7fffa0bc3130 I   /usr/share/cinnamon/js/ui/main.js:356 (d1ddd8d4b00 @ 87)

and

(cinnamon:4072): Cjs-CRITICAL **: 06:45:07.469: Attempting to run a JS callback during garbage collection. This is most likely caused by destroying a Clutter actor or GTK widget with ::destroy signal connected, or using the destroy(), dispose(), or remove() vfuncs. Because it would crash the application, it has been blocked.

(cinnamon:4072): Cjs-CRITICAL **: 06:45:07.469: The offending callback was SourceFunc().
== Stack trace for context 0x55a4aa7cd180 ==

@lestcape
Copy link

@fredcw I think that issue will be a little difficult to be fixed in cinnamon as is a gjs/cjs issue https://gitlab.gnome.org/GNOME/gjs/-/issues/327

@fredcw
Copy link
Contributor

fredcw commented Dec 19, 2020

@lestcape @mtwebster Adding:
const System = imports.system;
and
System.gc();
somewhere in _poll_timeout() seems to fix the problem but there's quite a performance hit. Maybe add a counter to call "System.gc();" every 100th time or something.

@mtwebster
Copy link
Member Author

mtwebster commented Dec 19, 2020

Weird, I have yet to run into this error. I wonder if I block that signal during a resize it might avoid it. Curious what part of the desktop is your menu applet on (bottom left, top left, etc..)?

edit: nm, I read the upstream issue, the timer probably what's causing this.

I can increase the timeout but it probably wouldn't fix this (and would make resizing ugly).

@fredcw
Copy link
Contributor

fredcw commented Dec 20, 2020

@mtwebster Not sure if my suggestion works or is even necessary as I've got it working in cinnamenu without using System.gc() linuxmint/cinnamon-spices-applets#3372

@lestcape
Copy link

@mtwebster Yes, the problem occurs in a callback in the upstream issue, so should be your timer here. Like @fredcw did not implement the timer in Cinnamenu, he does not see the problem in his solution. Anyway, i think you can not remove the timer because of #9771 (comment) and because update the setting in real time implies a bad performance.

@fredcw Yes a gc hide the problem, because reduce the possibility that an automatically gc will be called in the middle of a callback (the timer). But in a middle of an actor resize is not a good time to call a gc.

@mtwebster
Copy link
Member Author

I'm going to get rid of this timer, I don't want to be anywhere near that. This is going to wait until 4.0 anyhow.

@mtwebster mtwebster changed the title menu applet: Make the menu resizeable. [wip] menu applet: Make the menu resizeable. Dec 20, 2020
@lestcape
Copy link

lestcape commented Dec 25, 2020

Revisiting my own comment about the scroll of the favourite menu box, I need to add that even GNOME or Unity have the same problem and they don't show the scroll bar in his default left vertical panel. Also the cinnamon panel in the vertical or horizontal position have not a visible scroll. Probably this is why i dislike the idea of a visible scroll in the favourite box of the menu. I think in cases like that users are waiting to have a way to scroll, but not with a visible bar. The common way is just scrolling with the help of the mouse-over signal and not with the scroll bar. In Unity they implemented a revolutionary way to fold the elements, but this seem really difficult to be reproduced, anyway the Unity way was also a nice way in my opinion.

@fredcw
Copy link
Contributor

fredcw commented Dec 28, 2020

@mtwebster A strange thing: I just tried completely removing the time_out call in cinnamenu and replaced it with responding to all mouse events with Clutter.grab_pointer(this.actor) the same way js/ui/dnd.js does and cinnamon is still crashing when resizing in seemingly the same way is was before.

(cinnamon:1162): Cjs-ERROR **: 03:46:16.561: toggling down object GInotifyFileMonitor that's already queued to toggle up

Traceback (most recent call last):
  File "/usr/bin/cinnamon-launcher", line 72, in <module>
    os.execvp(FALLBACK_COMMAND, (FALLBACK_COMMAND,) + FALLBACK_ARGS)
  File "/usr/lib/python3.9/os.py", line 574, in execvp
    _execvpe(file, args)
  File "/usr/lib/python3.9/os.py", line 616, in _execvpe
    raise last_exc
  File "/usr/lib/python3.9/os.py", line 607, in _execvpe
    exec_func(fullname, *argrest)
FileNotFoundError: [Errno 2] No such file or directory

@fredcw
Copy link
Contributor

fredcw commented Jan 6, 2021

Not updating the settings-schema settings while resizing the menu (in cinnamenu) is the only solution I've found to this bug even when using Clutter.grab_pointer instead of the timer.

@lestcape
Copy link

lestcape commented Jan 6, 2021

First things first: Happy new year!

I think that the usage of cinnamon gsettings to globally store the menu size should probably be the best workaround to the problem right now. At least while the upstream gjs bug is resolved. This will also spam the disk, but the gsettings implementation should be much more robust than manually write a file in js. Well, at less i think it deserve test it.

Let me ask something out of scope here, only for curiosity: @mtwebster you are working on separate the shell ui from the muffin compositor? I found this code searching for input_shape_combine_region in github... This code can be used to separate the whole ui or just only the popupmenu or the component you want, as you can use GtkClutterEmbed in the transparent Gtk window with the same size as the whole gdk_display and build the Cinnamon ui inside the GtkClutterEmbed stage.

@lestcape
Copy link

lestcape commented Jan 7, 2021

I just reviewed the Configurable Menu code. What i was doing there was storage the menu position only in the on_mouse_released event (So, i have two set of variables to control the size. The settings ones and the current simples ones that are just used while the menu is alive). In that way the user can move the menu all he/she want and only when the menu resize is finished the result was storage.

This procedure reduce the disk load to the minimum and also is a work-around to the upstream bug, because a timer is not needed in that way. Also the solution ensure that the last menu position will be storaged, a thing that a timer can not ensure. The worse thing that can happens in that solution should be have a user making several clics inside the menu resize area. This is a thing that also should be fixed if we just chek if the size change really before the storage of the the values. Hope, this help.

@fredcw
Copy link
Contributor

fredcw commented Feb 4, 2021

This bug has cropped up in a couple of other places. One time when I was adjusting the panel label in settings, cinnamon crashed and again when adjusting the panel icon size (both times in cinnamenu).

@mtwebster
Copy link
Member Author

I merged a lot of things here into master, except for the actual dnd resizing. As a result, this probably isn't as urgent to handle, other than for the convenience factor. The menu now grows as needed, but can be fixed in height if desired - either way it enables scrollbars when necessary to keep all content accessible (and keep it from growing outside the work area).

@mtwebster
Copy link
Member Author

This isn't all that important at this point.

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.

6 participants