Menu was replaced with Gtk.Menu

avently commented 6 years ago

now menu has a style from theme
deepin-menu dependency was removed

It should be merged after CMakeLists PR since entry for a file /lib/menu.vala is located inside CMakeLists.txt and I removed that file.

manateelazycat commented 6 years ago

@avently I think this patch is not good enough since deepin-menu is standard module in deepin system.

Can you change patch that user can choose which menu through configure CMake ?

I want provide a option that user can use Gtk Menu if user not use deepin system, but i don't want remove deepin-menu from code, deepin user need it.

avently commented 6 years ago

OK, I'll do it.

manateelazycat commented 6 years ago

@avently Thanks a lot. ;)

avently commented 6 years ago

@manateelazycat

Can you change patch that user can choose which menu through configure CMake ?

Are you sure about CMake? Maybe better idea would be to change menu via settings? I don't believe someone will use CMake flag for building except developers.

manateelazycat commented 6 years ago

@avently Yes, you're right. We can build a new patch, deepin terminal choose GTK menu if it not found deepin-menu serivce in runtime.

Then user even don't need configure a settings, terminal popup deepin-menu if terminal found deepin-menu service, otherwise use GTK+ menu instead.

How do you think?

avently commented 6 years ago

Awesome idea. One moment, how about deepin-menu dependency in Arch and Ubuntu repositories? Will it be optional?

Also I'm experience problems with disconnecting from deepin-menu DBus service. How to properly disconnect Menu.Menu from next events? If I will make just menu = null; it will gives crashes in deepin-menu side.

avently commented 6 years ago

I added Gtk.Menu creation methods inside your Menu.vala. So now you can switch between these two menus. Implementation is pretty ugly now:

for some reason deepin-menu sometimes works sometimes not.
deepin-menu are not hiding with left mouse button click.
impossible to get Gtk.Menu working without hardcoding bool value here: https://github.com/linuxdeepin/deepin-terminal/pull/47/commits/3f3b2abaa98575cc03a02588aead79bf640a67e6#diff-5f0d4da3f49d84a42136ea015e898e76R60 So it doesn't happen automatically for now.
impossible to start menu without deepin-menu installed. It's behaviour somehow related to DBus starting process (if service is not found Menu object is null). I didn't work with DBus earlier so I don't know how to work with situation when DBus can't find a service.

manateelazycat commented 6 years ago

@avently For found deepin-menu serive, you can use below code for automatically test:

try { MenuManagerInterface menu_manager_interface = Bus.get_proxy_sync(BusType.SESSION, "com.deepin.menu", "/com/deepin/menu"); string menu_object_path = menu_manager_interface.RegisterMenu(); return true; } catch (Error e) { // deepin-menu service is not support by DBus if above code throw a exception. return false; }

You can build a test function when user right-click to call menu, if return true call deepin-menu code, otherwise call GTK+ menu.

You can use command "sudo mv /usr/bin/deepin-menu /usr/bin/deepin-test" to test above code.

avently commented 6 years ago

Ok, I found the cause of some issues. Added new commit with fixes. The problem was with catch{} block. It used IOError instead of Error. Also I installed original Deepin Terminal from Arch repos and found the same issues I posted (focus lost doesn't hide deepin-menu, sometimes deepin-menu doesn't response at all to DBus message). So it is not related to my code.

Please, review the PR. Looks not so bad I hope:)

manateelazycat commented 6 years ago

@avently After i apply your patch, have many new bugs.

After i check code, i found implement is little bit ugly. Specify is_gtk_menu is something hacking way, I will write new patch implement your idea with big refacotry.

Thank you very much for effort, but patch is looks not so good, i can't merge it, sorry.

avently commented 6 years ago

@manateelazycat ok. Gtk part of code is right and I like it but I don't like how I structured menu.vala. Will you get my code and will make big refactoring? I spent many hours to figure out how to make gtk styles correctly so it may be useful. Also when do you plan to implement this feature?

manateelazycat commented 6 years ago

@avently Since GTK only supports integral scaling, GTK+/Wayland has support float scaling, but GTK+/Xorg looks not plan to support float scaling that deepin-terminal is very bad effect for HiDPI.

I recently plan to sent time to rewrite deepin-terminal with QT to fix float scaling for HiDPI.

I'm happy to apply your rest features, such as resizable workspaces.

I can't promise time on implement deepin-menu/gtk autoselect, i suggestion you use your first solution (remove menu.vala) for your need.

martyr-deepin / deepin-terminal-gtk

Menu was replaced with Gtk.Menu #47