Skip to content
This repository has been archived by the owner on Sep 2, 2021. It is now read-only.

Linux native menus checked entries #633

Merged
merged 10 commits into from
Mar 8, 2018

Conversation

pelatx
Copy link
Contributor

@pelatx pelatx commented Feb 19, 2018

This PR adds functionality so that entries in the native Linux menus reflect their checked / unchecked status.

@pelatx pelatx changed the title Checked menu entries working. Linux native menus checked entries Feb 19, 2018
@nethip
Copy link
Contributor

nethip commented Feb 26, 2018

@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?

@pelatx
Copy link
Contributor Author

pelatx commented Feb 26, 2018

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.

@nethip
Copy link
Contributor

nethip commented Feb 27, 2018

@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 👍

@nethip
Copy link
Contributor

nethip commented Feb 27, 2018

@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.

@pelatx
Copy link
Contributor Author

pelatx commented Feb 27, 2018

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.

@pelatx
Copy link
Contributor Author

pelatx commented Feb 28, 2018

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.
To avoid that was the reason why this PR turned out to be a little tangled. 😅

checkboxes-brackets

@nethip
Copy link
Contributor

nethip commented Feb 28, 2018

@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?

@pelatx
Copy link
Contributor Author

pelatx commented Feb 28, 2018

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.

@pelatx
Copy link
Contributor Author

pelatx commented Mar 2, 2018

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?

@nethip
Copy link
Contributor

nethip commented Mar 5, 2018

Thanks @pelatx! Apologies for not being able to respond to you. I will review this in a day or two.

@pelatx
Copy link
Contributor Author

pelatx commented Mar 5, 2018

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.

}
index++;
} while ((children = g_list_next(children)) != NULL);
g_list_free(children);
Copy link
Contributor

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?

@nethip
Copy link
Contributor

nethip commented Mar 7, 2018

@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 👏

@pelatx
Copy link
Contributor Author

pelatx commented Mar 7, 2018

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.

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);
Copy link
Contributor

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.

@nethip
Copy link
Contributor

nethip commented Mar 8, 2018

@pelatx the changes look absolutely fine! Just add some NULL checks pointed above, so that I can go ahead with the merge.

@nethip
Copy link
Contributor

nethip commented Mar 8, 2018

Thanks @pelatx . Awesome job 👍

@nethip nethip merged commit ce6fc09 into adobe:master Mar 8, 2018
@pelatx pelatx deleted the linux-native-menus-checked-state branch March 14, 2018 01:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants