core-ai-bot
commented
2 years ago 
@pelatx Thanks for this PR! I will review this. A quick comment. I see that the unit tests are missing for this PR. Could you add unit tests for the new code?
Open core-ai-bot opened 2 years ago
core-ai-bot
commented
2 years ago
@pelatx Thanks for this PR! I will review this. A quick comment. I see that the unit tests are missing for this PR. Could you add unit tests for the new code?
core-ai-bot
commented
2 years ago 
I'm sorry @nethip . I'm completely new programming in C/C++. I had to learn the basics of GTK+ and get acquainted with the Brackets-shell code to write this. But I do not know how to write unit tests following the methodology you use in this project.
Maybe you could give me some guidance to know where to start.
Thank you.
core-ai-bot
commented
2 years ago
@pelatx No issues! We can guide you through the process. Please refer to https://github.com/adobe/brackets-shell/wiki. This should be a good place to get started.
About unit tests: I just looked at what needs to be written/enabled. We don't have to write any new unit tests as most of it is covered in existing tests. Please refer to this piece of code.
https://github.com/adobe/brackets/blob/master/test/spec/NativeMenu-test.js#L1291
All of our existing tests can be found in Brackets repo at https://github.com/adobe/brackets/tree/master/test
And good job on the PR :+1:
core-ai-bot
commented
2 years ago
@pelatx I have reviewed your code and I think we can simplify it much further.
I have created a branch for Linux enhancements and pushed some changes relating to check marks of menus on Linux. Would you take a look at it and see if the solution is simple? I did not unit test this on Debian though.
https://github.com/adobe/brackets-shell/compare/nethip/LinuxEnhancements
Quite a no of things to consider.
1) Upon calling gtk_check_menu_item_set_active, menu activation was getting triggered because of which a psuedo command firing was happening. So had to restrict that here
2) With the check marked menus, just upon clicking the menus, the check mark was getting toggled by default. So kind of handled this too here. Since this is a hack, need to verify on Debian and Ubuntu 17.0.
Let me know if it is OK for you to contribute to branch nethip/LinuxEnhancements. We still have some couple of enhancements on Linux side. It would be great if you are willing to have this.
core-ai-bot
commented
2 years ago
Wow, you have eliminated in a moment several problems that I found when I wrote this and that they complicated it a lot.
If I understand it well, with those changes there will be no need to call the callback of a check menu item twice and this can be simplified.
On the other hand, I tried another solution at the beginning, which was based on replacing gtkMenuItem with gtkCheckMenuItem in AddMenuItem (), as I see in the LinuxEnhancements branch. But I did not like that all the items in the menus appear with the check box if they do not really have a toggle function. This would be the view we would have, right?
I agree with what you propose @nethip. I will try to contribute what I can.
Later I will try to test these changes in Debian and adapt the PR code.
Thank you very much for the help about the unit tests.
core-ai-bot
commented
2 years ago
I have not been successful compiling the LinuxEnhancements branch in Debian 9. But in Debian 8.
Your changes make everything work perfectly. :clap::clap::clap:
The only thing that still does not like is that we see the check box in menu entries that are not switchable. To avoid that was the reason why this PR turned out to be a little tangled. :sweat_smile:
core-ai-bot
commented
2 years ago
@pelatx Ah huh! I get it now. So the checkbox is getting displayed irrespective of wheter the menu is switchable or not. So the way you fixed this is by recreating a menu each time a menu item state set has been requested.
I think we can go ahead tweaking this PR itself and merging some of my changes with this. Can you tweak on the menu re-creation side a little while I can contribute with other related things, like blocking the menu command recreation when not required e.t.c?
core-ai-bot
commented
2 years ago
Of course, yes, I had already decided to adapt it and show it to you later, so that you can consider it.
With your hack, I think that the callback for the GtkCheckMenuItem can be simplified.
This PR works by creating a GtkCheckMenuItem pair of the original item the first time it is checked. And hiding the original item. Then we only play with the visibility of one and the other. The entire menu is not recreated each time.
The corresponding GtkCheckMenuItem callback activates the original item and thus the checked item is transparent in terms of functionality. It is a purely visual element.
I will try to adapt it to the branch and complete it by modifying removeMenuItem () as well, so that the GtkCheckMenuItem is eliminated when the original is. Which I had overlooked.
core-ai-bot
commented
2 years ago
I had compiled and tested the PR (as it was before this last commit) only on linux-1547 branch. Later I have seen that the same code does not work on master. Sorry for this.
That is why I have searched for another way to obtain the checked items and it has resulted in a simpler code.
Please, could you review it @nethip?
core-ai-bot
commented
2 years ago
Thanks @pelatx! Apologies for not being able to respond to you. I will review this in a day or two.
core-ai-bot
commented
2 years ago
No problem @nethip.
Actually this PR, in general, works as expected.
In the case of the three menu items related to split the view, they work well when we select a different one from the one currently selected. If we select the same, the CheckMenuItem will be unchecked but it will not be changed by a MenuItem. It seems that, by acting as a group of radio buttons, the SetMenuItemState() function is not called in this case.
To solve this, the hack you made is perfect. I have included my code in your LinuxEnhancements branch. And I added a condition here to avoid the cast errors that occur when the function receives a MenuItem instead of a CheckMenuItem.
In this way, it seems that everything works well (on Debian 8 at least).
Edit: the unit tests related to the native menus are giving me nine failures of twenty-eight over the LinuxEnhancements branch with my changes. Then I tried the unit tests on master and it gives me the same errors. Maybe I'm doing something wrong.
core-ai-bot
commented
2 years ago
@pelatx Awesome! This works as expected.
I checked this on both Ubuntu 16 and Debian 9 and they seem to be working fine there. I don't think you would require any of the hacks that I had in my LinuxEnahacements branch. If you think you require any, please update this PR with them and I will check and merge this PR.
I found a small bug, with which we can live as well actually. Upon clicking the No Split menu entry, the box stays there.
About unit tests: There may be some unit tests failing already. They may not be related to this PR. It would be great if you can have a look at some. If not we can go ahead merging this PR and take a look at the unit test case failures later, as those failures are not related to this PR.
Good job :clap:
core-ai-bot
commented
2 years ago
Thanks @nethip.
I also think that the failures in unit tests are not related to this PR. Exactly the same failures have arisen when I tried in Master branch.
Your hack fixes precisely the small bug of No Split. Later I will update this PR so you can check it.
core-ai-bot
commented
2 years ago
@pelatx the changes look absolutely fine! Just add some NULL checks pointed above, so that I can go ahead with the merge.
core-ai-bot
commented
2 years ago
This PR adds functionality so that entries in the native Linux menus reflect their checked / unchecked status.
pelatx included the following code: https://github.com/adobe/brackets-shell/pull/633/commits