mate-desktop / marco

MATE default window manager
https://mate-desktop.org
GNU General Public License v2.0
193 stars 86 forks source link

Safeguard against calling gdk_x11_window_get_xid with a NULL GdkWindow #700

Closed rcaridade145 closed 2 years ago

rcaridade145 commented 2 years ago

This pull request aims to fix #692

zhuyaliang commented 2 years ago

The source code format should be maintained

rcaridade145 commented 2 years ago

Replace tabs with spaces.

raveit65 commented 2 years ago

Thank you, this looks better, now a review can started.

zhuyaliang commented 2 years ago

@rcaridade145 I think we only need to add three lines of code, so that the modification of the existing code is the least

       if (ok1 && popup_widget != NULL)
         {
+          GdkWindow * window=gtk_widget_get_window (popup_widget);
+          if (window == NULL)
+              return FALSE;
           Window popup_xid = gdk_x11_window_get_xid (gtk_widget_get_window (popup_widget));

           gboolean ok2 = XTranslateCoordinates (display->xdisplay,

Of course, there is no problem with your code. I will respect your code style

rcaridade145 commented 2 years ago

https://hackerchick.com/religious-war-48293-single-vs-multiple/ I prefer a single one.

zhuyaliang commented 2 years ago

@rcaridade145 OK, but the code format needs to be modified and some spaces need to be added

tormodvolden commented 2 years ago

The established code style in this file is to use early, multiple returns when possible, instead of else clauses and the extra indentation it entails. IMO consistent code style is more important than individual developer preference.

rcaridade145 commented 2 years ago

Will change the code.

tormodvolden commented 2 years ago

The whitespace police will arrest you for window=gtk_widget_get_window. It can also be argued that all declarations such as Window popup_xid should be at the start of the block and not intermixed with statements.

rcaridade145 commented 2 years ago

I've added spaces.

raveit65 commented 2 years ago

@zhuyaliang When 2 or 3 reviewers (with you) did approved a normal PR, it is save to merge. Everybody from the team is allowed to do that....

raveit65 commented 2 years ago

@rcaridade145 Another code-formatting issue of your small change with 4 lines of code. https://github.com/mate-desktop/marco/commit/a905a4d11aa25be55bbe3b22d9c7b23b46a50e0c

tormodvolden commented 2 years ago

To everybody's defense such trailing whitespace doesn't not show up in Github's web view, they seem to strip it off. Bad Github. But as a tip, it does show up (in red) with "git diff" before the commit and with "git show" before you push the commit anywhere.