mate-desktop / mate-panel

MATE panel
https://mate-desktop.org
GNU General Public License v2.0
185 stars 118 forks source link

button-widget.c: replace tabs with 4 spaces #1390

Closed lukefromdc closed 1 year ago

lukefromdc commented 1 year ago

*and reformat as necessary

lukefromdc commented 1 year ago

One possible way of doing this, I am open to other options as well. Objective is easier to read and maintain code. This is a reformat of white space only. Should be NO changes to built binary and NO change to anything but a tab, a space, or in a few cases a blank line or an opening bracket position.

A test built and run gave no surprises and identical runtime behavior in both x11 and Wayland, ANY difference would have indicated a typo somewhere. We need to make sure no typos get in with this sort of reformat, note that GNOME did this with an automated tool some years ago, using a 2 spaces instead of tabs format. Most new code in MATE we use four spaces so that's what I used here. Original tabs appear to have been intended for 8 spaces but little further work was needed to line everything up with 4 spaces, which saves space and matches most of our newer work.

lukefromdc commented 1 year ago

@raveit65 , if you want to do this yourself in a different way, feel free to either force-push to this branch, or open another PR and I will close this one in preference to it as you are probably our best formatter. Had some time so decided this might save you some.

raveit65 commented 1 year ago

Cool, i will take a look at it later and push changes for myself to this branch.

raveit65 commented 1 year ago

Seems like the file was a mix of tab-size=4/8. o_0 No wonder that a fine formatting was impossible. Using spaces everywhere and 4 spaces for indents will terminate this confusion.

raveit65 commented 1 year ago

I'd like to take another look and do a runtime test before review and merging.

lukefromdc commented 1 year ago

Those are single-line if clauses w no brackets, thus the blank lines beneath

On 7/22/2023 at 3:58 PM, "raveit65" @.***> wrote:

@raveit65 commented on this pull request.

  • if (button->priv->surface)
  • cairo_surface_destroy (button->priv->surface);
  • if (button->priv->surface_hc)
  • cairo_surface_destroy (button->priv->surface_hc);
  • button->priv->surface_hc = NULL;
  • button->priv->surface = NULL;
  • if (button->priv->surface_hc)
  • cairo_surface_destroy (button->priv->surface_hc);
  • button->priv->surface_hc = NULL;

@mate-desktop/core-team Not sure about adding a blank line after the if clauses. button->priv->surface = NULL; and button->priv->surface_hc = NULL; doesn't belongs to the if clauses?

-- Reply to this email directly or view it on GitHub: https://github.com/mate-desktop/mate- panel/pull/1390#pullrequestreview-1542137707 You are receiving this because you authored the thread.

Message ID: <mate-desktop/mate- @.***>

raveit65 commented 1 year ago

I tested that change with x11 and wayland. Everything works as expected. Please review. Commits can be squashed in one during merge.

lukefromdc commented 1 year ago

I am on the road now away from my desktop, will look it over when I get home. Will also build and install, testing in both x11 and wayland

On 7/22/2023 at 6:13 PM, "raveit65" @.***> wrote:

I tested that change with x11 and wayland. Everything works as expected. Please review. Commits can be squashed in one during merge.

-- Reply to this email directly or view it on GitHub: https://github.com/mate-desktop/mate-panel/pull/1390#issuecomment- 1646681314 You are receiving this because you authored the thread.

Message ID: <mate-desktop/mate- @.***>