-
Notifications
You must be signed in to change notification settings - Fork 617
Conversation
…and the checked one.
…ems when the one that is ckecked at that moment is clicked.
…called with checked flag to true and reutilized later.
@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? |
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. |
@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 👍 |
@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.
Let me know if it is OK for you to contribute to branch |
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. 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. |
I have not been successful compiling the LinuxEnhancements branch in Debian 9. But in Debian 8. Your changes make everything work perfectly. 👏👏👏 The only thing that still does not like is that we see the check box in menu entries that are not switchable. |
@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? |
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. |
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? |
Thanks @pelatx! Apologies for not being able to respond to you. I will review this in a day or two. |
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. 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. |
appshell/appshell_extensions_gtk.cpp
Outdated
} | ||
index++; | ||
} while ((children = g_list_next(children)) != NULL); | ||
g_list_free(children); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it neccessary to free the list?
@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 I found a small bug, with which we can live as well actually. Upon clicking the 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 👏 |
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 |
…`, etc. And addressing review comments.
appshell/appshell_extensions_gtk.cpp
Outdated
NativeMenuModel& model = NativeMenuModel::getInstance(getMenuParent(browser)); | ||
int tag = model.getTag(command); | ||
if (tag == kTagNotFound) { | ||
return ERR_NOT_FOUND; | ||
} | ||
GtkWidget* menuItem = (GtkWidget*) model.getOsItem(tag); | ||
gtk_widget_set_sensitive(menuItem, enabled); | ||
GtkWidget* parent = gtk_widget_get_parent(menuItem); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When dealing with pointer it is always good to check for NULL
pointers, even if it is obvious. That ways the we can avoid an app crash.
@pelatx the changes look absolutely fine! Just add some |
Thanks @pelatx . Awesome job 👍 |
This PR adds functionality so that entries in the native Linux menus reflect their checked / unchecked status.