p-e-w / finalterm

At last – a modern terminal emulator (NO LONGER MAINTAINED)
http://finalterm.org
GNU General Public License v3.0
3.84k stars 179 forks source link

Fix #187 by removing / readding menu_button on mouse_enter/leave events #367

Closed arkocal closed 9 years ago

arkocal commented 9 years ago

See commit message

p-e-w commented 9 years ago

This looks good! Indeed, your solution should allow us to get rid of lots more boilerplate: For example, it seems to me that the following code parts are now all unnecessary and can be removed: https://github.com/arkocal/finalterm/blob/Iss187/src/TerminalView.vala#L189, https://github.com/arkocal/finalterm/blob/Iss187/src/TerminalView.vala#L199, https://github.com/arkocal/finalterm/blob/Iss187/src/TerminalView.vala#L179-L182, https://github.com/arkocal/finalterm/blob/Iss187/src/TerminalView.vala#L322.

https://github.com/arkocal/finalterm/blob/Iss187/src/TerminalView.vala#L229 probably needs to be adapted to the new visibility paradigm as well.

Also, could you please fix the indentation to be in line with the rest of the code? You can see the issues by switching to the "Files changed" tab in this PR. Just use tabs everywhere and you'll be fine :)

arkocal commented 9 years ago

This should work fine now. I have realized that the older commit sometimes did not work because click event and leave event are triggered in different orders, hence the check. Tabs should also be OK, I had just forgotten to set my editor to tabs instead of spaces.

p-e-w commented 9 years ago

I'd really like to merge this PR since it fixes an annoying problem but your comments about the AutoComplete window make me think you still want to change something. If you want me to merge now please squash the commits into one and we can revisit that issue later. Otherwise, I'll wait of course.

arkocal commented 9 years ago

I was actually thinking of fixing that, but because there is no direct connection between the autocomplete and terminalview classes, I could not find a clean solution to that. You can very well merge this, the autocomplete issue actually another issue. I can not squash these commits because they are online, should I squash them into main and make another pull request?

p-e-w commented 9 years ago

You can squash them "online" as well, no problem: Just squash locally and force-push to arkocal:Iss187, this pull request will then automatically update. Yes, GitHub is awesome :smiley:

arkocal commented 9 years ago

I hope it is good now :)

p-e-w commented 9 years ago

Sorry, but while squashing you seem to have undone your last commit :( I now see all the above mentioned issues again, such as the blank line, the superfluous line 322, and the problem in line 228.

arkocal commented 9 years ago

Luckly I had a recent system backup. I have checked for the named issues, compiled and retested because I was not quite sure if that was the latest version. Seems good to me, I wonder what I am missing this time :smile:

p-e-w commented 9 years ago

This time everything worked. Merged! Your name is now in the about dialog credits.

The original issue is definitely fixed. One thing I noticed is that when toggling and then un-toggling the button, I get

Clutter-WARNING **: Attempting to remove actor of type 'MxButton' from group of class 'ClutterActor', but the container is not the actor's parent.

Also, untoggling still doesn't work as expected (removes the button rather than untoggling it), but that was already an issue before of course.

Thank you for your contribution! :fireworks: