avently
commented
5 years ago @BLumia and @hualet what do you think about this PR?
Closed avently closed 4 years ago
avently
commented
5 years ago @BLumia and @hualet what do you think about this PR?
BLumia
commented
5 years ago Did a quick look and it looks pretty good to me. Hope @hualet can also take a look at the patch since I'm not good at coding in vala :-/
BLumia
commented
4 years ago Hi and I just tested your patch (I manually rebased your patch onto the current master and did a quick test on my local machine) under a weston wayland session(switch to a new tty and make sure XDG_RUNTIME_DIR is set properly, then just run weston, a DRM mode weston wayland session will be ready to use), and here are some issue I've got.
Couldn't map as window %p as popup because it doesn't have a parent warning). IMO we can ignore the given position and use popup_at_pointer() directly, since the given position is because the position is required for passing it to deepin-menu.And for your questions:
also I think we should remove the top right menu because it useless (except Start new terminal and About options but we can move them into right click menu)
It's the designer's decition so we'd better leave the humberger menu as-is. A config option to hide it is okay for me ;)
do I need to make submenu = null? Or memory will be freed automatically since I don't have hard references on submenus?
You don't need to do submenu = null manually :)
BLumia
commented
4 years ago Hi and since I need to get it working under wayland without deepin-menu now, so I submited a commit (https://github.com/linuxdeepin/deepin-terminal/commit/1d3a33937f8f49d654f654a8fe16e0380c0d36c4) which included similar behavior as this PR, you can take a look at that commit to see the differences.
That commit doesn't comes with the context menu theme support but it should be very easy to add by modify menu.vala :)
avently
commented
4 years ago @BLumia ok, I'll rebase my changes then. I can't test Wayland support because sway and weston unable to run a desktop on my Nvidia card (something related to DRM, didn't googled this error yet).
avently
commented
4 years ago Ok, done. It was terribly easy after the changes you made. Thanks for allowing GtkMenu support!
BLumia
commented
4 years ago The new patch looks good to me and it no longer have the background color issue. The four corner of the context menu have a white spot outside the round corner but I'm not sure about if it's a render issue on my laptop. I'm not sure if it's okay to merge this one so I hope @hualet can also take a look.
btw, I think you can submit the change in widget/terminal.vala in another pull request since it's another bug which not quite related to GTK Menu styling, and this one is what I'm pretty sure I can get it merge right away :D
avently
commented
4 years ago The four corner of the context menu have a white spot outside the round corner
I don't have anything like that with light & dark themes. I re-uploaded my code with some changes. Is it looking better then before? if not, try to change your system-wide theme (maybe it adds some styling to menus).
I think you can submit the change in widget/terminal.vala in another pull request since it's another bug
I just changed a place of one line of code. I hooope you are joking when saying about new PR for that line. Because if you was serious... I know nothing about managing a project in this case.
P.S. made menu opacity = 0.95. Looks better with compositor than before
BLumia
commented
4 years ago I just changed a place of one line of code. I hooope you are joking when saying about new PR for that line. Because if you was serious... I know nothing about managing a project in this case.
No i'm not, it called Atomic Commit which means do one simple thing in every commit. It can make code review much easier. And since it's a single line change so it could be very simple to submit another PR for that change which I can get it merge right away instead of wait for the code review of the rest part of this pull request :)
avently
commented
4 years ago The idea of Atomic Commit is to make life easier. If you are reviewing two PRs instead of one PR + one changed line != to make life easier. You need to read the description of two PRs instead of one, browse a code, recheck if it works fine or not. BLumia, one line of code! You spend some minutes for answering to me instead of accepting the one-line change. :)
instead of wait for the code review of the rest part of this pull request
I waited since October. I can live without fast review. It makes me less happy and makes me less motivated to open new PRs but yeah, my life continues.
BLumia
commented
4 years ago If you are reviewing two PRs instead of one PR + one changed line != to make life easier.
In current state, it's still one single PR, not plus one line change, sine it's still in the only one PR.
You spend some minutes for answering to me instead of accepting the one-line change. :)
Actually I accept that one-line change (that's why I say I can get it merge right away if you can make it into a separate PR) but I cannot just merge that one-line part only and leave the rest of code in this PR not merged. All I can do is merge this PR (which I'm not quite sure if its okay) or not merge this PR. That's how git works :(
I can submit that change by myself but I think it's more about your contribution and it's very easy to submit another PR which just cost few minutes to do...
btw, if you did that change in two different commit but it's in one PR, it's still okay since I can simply cherry-pick that commit to master branch, but in current state it's also in a single commit...
avently
commented
4 years ago I can live without arguing about this little change. Thank you for understanding.
All I can do is merge this PR (which I'm not quite sure if its okay)
I'll be here when you make this decision.
avently
commented
4 years ago Force-pushed without the line. Now you are free to do with this PR whatever you want
BLumia
commented
4 years ago I'll be here when you make this decision.
Still, @hualet could you please take a look?
felixonmars
commented
4 years ago Thanks for this and I'm going to tag a new release.
Hello. This is a new version of my old PR related to adding support of Gtk menu. Deepin menu is working fine for those users who has /usr/bin/deepin-menu dependency installed. So if you have deepin-menu you'll see deepin menu. Otherwise you'll see Gtk implementation of menu.
To check this PR remove deepin menu (you don't even need to restart your terminal) and make a right click of a mouse. Gtk implementation will appear. To return things back install deepin menu again.
If you're asking why it's needed:
I tried to make the code look clean but if you have another vision of a good code tell me and I'll make it better.
What's good and what's bad in the PR:
coloring
universal submenu
menu in settings
menu under the top right button has bad positioning (as well as with tabbar_at_the_bottom option enabled). I don't now how to get correct position for the menu close to the button location. Will investigate it
also I think we should remove the top right menu because it useless (except Start new terminal and About options but we can move them into right click menu)
I don't have anything except XFCE and i3wm so I'm not sure it works everywhere
do I need to make submenu = null? Or memory will be freed automatically since I don't have hard references on submenus?
Share your thoughts about this PR, please. Also I think we can remove deepin-menu dependency from packages afterwards because Deepin users will have it anyway from DE components.
P.S. For now It's not ready for merging because of the right menu location.