[PATCH] wmaker: src/action.c merge duplicate code

[David Maciejak]

This patch is merging some duplicate code related
to animation position (I also fixed most of the errors reported by checkpatch).

---
 src/actions.c | 110 ++
 1 file changed, 50 insertions(+), 60 deletions(-)

diff --git a/src/actions.c b/src/actions.c
index ae326a1..8a5ff82 100644
--- a/src/actions.c
+++ b/src/actions.c
@@ -60,12 +60,12 @@ static struct {
  int steps;
  int delay;
 } shadePars[5] = {
- {
- SHADE_STEPS_UF, SHADE_DELAY_UF}, {
- SHADE_STEPS_F, SHADE_DELAY_F}, {
- SHADE_STEPS_M, SHADE_DELAY_M}, {
- SHADE_STEPS_S, SHADE_DELAY_S}, {
-SHADE_STEPS_US, SHADE_DELAY_US}};
+ { SHADE_STEPS_UF, SHADE_DELAY_UF },
+ { SHADE_STEPS_F, SHADE_DELAY_F },
+ { SHADE_STEPS_M, SHADE_DELAY_M },
+ { SHADE_STEPS_S, SHADE_DELAY_S },
+ { SHADE_STEPS_US, SHADE_DELAY_US }
+};

 #define UNSHADE 0
 #define SHADE   1
@@ -84,7 +84,8 @@ static int compareTimes(Time t1, Time t2)
 #ifdef ANIMATIONS
 static void shade_animate(WWindow *wwin, Bool what);
 #else
-static inline void shade_animate(WWindow *wwin, Bool what) {
+static inline void shade_animate(WWindow *wwin, Bool what)
+{
  /*
  * This function is empty on purpose, so tell the compiler
  * to not warn about parameters being not used
@@ -291,9 +292,9 @@ void wUnshadeWindow(WWindow *wwin)
 static void save_old_geometry(WWindow *wwin, int directions)
 {
  /* never been saved? */
- if (! wwin->old_geometry.width)
+ if (!wwin->old_geometry.width)
  directions |= SAVE_GEOMETRY_X | SAVE_GEOMETRY_WIDTH;
- if (! wwin->old_geometry.height)
+ if (!wwin->old_geometry.height)
  directions |= SAVE_GEOMETRY_Y | SAVE_GEOMETRY_HEIGHT;

  if (directions & SAVE_GEOMETRY_X)
@@ -584,8 +585,7 @@ static void find_Maximus_geometry(WWindow *wwin,
WArea usableArea, int *new_x, i
  remember_geometry(wwin, &orig.left, &orig.top, &orig.width, &orig.height);
  orig.bottom = orig.top + orig.height;
  orig.right = orig.left + orig.width;
- }
- else
+ } else
  set_window_coords(wwin, &orig);

  /* Try to fully maximize first, then readjust later */
@@ -862,6 +862,7 @@ animateResizeTwist(WScreen *scr, int x, int y, int
w, int h, int fx, int fy, int
  points[3].y = cy + sin(angle + a + WM_PI) * d;
  points[4].x = cx + cos(angle - a) * d;
  points[4].y = cy + sin(angle - a) * d;
+
  XGrabServer(dpy);
  XDrawLines(dpy, scr->root_win, scr->frame_gc, points, 5, CoordModeOrigin);
  XFlush(dpy);
@@ -1038,7 +1039,7 @@ static void mapTransientsFor(WWindow *wwin)
  }
 }

-static WWindow *recursiveTransientFor(WWindow * wwin)
+static WWindow *recursiveTransientFor(WWindow *wwin)
 {
  int i;

@@ -1060,7 +1061,35 @@ static WWindow *recursiveTransientFor(WWindow * wwin)
  return wwin;
 }

-void wIconifyWindow(WWindow * wwin)
+static int getAnimationGeometry(WWindow *wwin, int *ix, int *iy, int
*iw, int *ih)
+{
+ if (!wwin->screen_ptr->flags.startup && !wPreferences.no_animations
+ && !wwin->flags.skip_next_animation && wwin->icon != NULL) {
+ if (!wPreferences.disable_miniwindows
+&& !wwin->flags.net_handle_icon) {
+ *ix = wwin->icon_x;
+ *iy = wwin->icon_y;
+ *iw = wwin->icon->core->width;
+ *ih = wwin->icon->core->height;
+ } else {
+ if (wwin->flags.net_handle_icon) {
+ *ix = wwin->icon_x;
+ *iy = wwin->icon_y;
+ *iw = wwin->icon_w;
+ *ih = wwin->icon_h;
+ } else {
+ *ix = 0;
+ *iy = 0;
+ *iw = wwin->screen_ptr->scr_width;
+ *ih = wwin->screen_ptr->scr_height;
+ }
+ }
+ return 1;
+ }
+ return 0;
+}
+
+void wIconifyWindow(WWindow *wwin)
 {
  XWindowAttributes attribs;
  int present;
@@ -1137,6 +1166,9 @@ void wIconifyWindow(WWindow * wwin)
  unmapTransientsFor(wwin);

  if (present) {
+#ifdef ANIMATIONS
+ int ix, iy, iw, ih;
+#endif
  XUngrabPointer(dpy, CurrentTime);
  wWindowUnmap(wwin);
  /* let all Expose events arrive so that we can repaint
@@ -1150,28 +1182,7 @@ void wIconifyWindow(WWindow * wwin)

  flushExpose();
 #ifdef ANIMATIONS
- if (!wwin->screen_ptr->flags.startup && !wwin->flags.skip_next_animation
-&& !wPreferences.no_animations) {
- int ix, iy, iw, ih;
-
- if (!wPreferences.disable_miniwindows && !wwin->flags.net_handle_icon) {
- ix = wwin->icon_x;
- iy = wwin->icon_y;
- iw = wwin->icon->core->width;
- ih = wwin->icon->core->height;
- } else {
- if (wwin->flags.net_handle_icon) {
- ix = wwin->icon_x;
- iy = wwin->icon_y;
- iw = wwin->icon_w;
- ih = wwin->icon_h;
- } else {
- ix = 0;
- iy = 0;
- iw = wwin->screen_ptr->scr_width;
- ih = wwin->screen_ptr->scr_height;
- }
- }
+ if (getAnimationGeometry(wwin, &ix, &iy, &iw, &ih)) {
  animateResize(wwin->screen_ptr, wwin->frame_x, wwin->frame_y,
   wwin->frame->core->width, wwin->frame->core->height, ix, iy, iw, ih);
  }
@@ -1291,34 +1302,13 @@ void wDeiconifyWindow(WWindow *wwin)
  /* if the window is in another workspace, do it silently */
  if (!netwm_hidden) {
 #ifdef ANIMATIONS
- if (!wwin->screen_ptr->flags.startup && !wPreferences.no_animations
-&& !wwin->flags.skip_next_animation && wwin->icon != NULL) {
- int ix, iy, iw, ih;
-
- if (!wPreferences.disable_m

Re: [PATCH] wmaker: src/action.c merge duplicate code

[Carlos R. Mafra]

On Fri, 22 Aug 2014 at 15:22:16 +0800, David Maciejak wrote:
> This patch is merging some duplicate code related
> to animation position (I also fixed most of the errors reported by 
> checkpatch).

Sorry, I should have said this in your earlier patch.

Don't mix substantial coding style changes with new code, that's a recipe
to make things hard to debug.

Just suppose for a moment that someone finds that this patch causes a problem
(e.g. using 'git bisect'). Now one needs to understand what in this patch
causes the problem and the amount of unrelated changes make the task a pain.

And if we revert the patch, all the coding style changes will be gone too but
they had nothing to do with the problem. That's a clear indication that the
patch is doing 'more than one thing' at a time, which is not good.

Sorry, but I cannot accept the patch right now. I suggest to split it; one
doing purely style changes and the other merging the code.


-- 
To unsubscribe, send mail to wmaker-dev-unsubscr...@lists.windowmaker.org.

Re: [PATCH] wmaker: src/action.c merge duplicate code

[David Maciejak]

No problem i understand, enclosed the new patch.

On Fri, Aug 22, 2014 at 6:55 PM, Carlos R. Mafra  wrote:
>
> On Fri, 22 Aug 2014 at 15:22:16 +0800, David Maciejak wrote:
>> This patch is merging some duplicate code related
>> to animation position (I also fixed most of the errors reported by 
>> checkpatch).
>
> Sorry, I should have said this in your earlier patch.
>
> Don't mix substantial coding style changes with new code, that's a recipe
> to make things hard to debug.
>
> Just suppose for a moment that someone finds that this patch causes a problem
> (e.g. using 'git bisect'). Now one needs to understand what in this patch
> causes the problem and the amount of unrelated changes make the task a pain.
>
> And if we revert the patch, all the coding style changes will be gone too but
> they had nothing to do with the problem. That's a clear indication that the
> patch is doing 'more than one thing' at a time, which is not good.
>
> Sorry, but I cannot accept the patch right now. I suggest to split it; one
> doing purely style changes and the other merging the code.
>
>
> --
> To unsubscribe, send mail to wmaker-dev-unsubscr...@lists.windowmaker.org.


0001-wmaker-src-action.c-merge-duplicate-code.patch
Description: Binary data

Re: [PATCH] wmaker: src/action.c merge duplicate code

[Rodolfo García Peñas (kix)]

David Maciejak  escribió:


No problem i understand, enclosed the new patch.


Hi David,

is possible remove the extra curly brackets (in both "if"s)?:

+   if (getAnimationGeometry(wwin, &ix, &iy, &iw, &ih)) *{*
animateResize(wwin->screen_ptr, ix, iy, iw, ih,
  wwin->frame_x, wwin->frame_y,
  wwin->frame->core->width,
wwin->frame->core->height);
*}*


Thanks a lot for your patches.
Best regards,
Rodolfo.
Rodolfo García Peñas (kix)
http://www.kix.es/


--
To unsubscribe, send mail to wmaker-dev-unsubscr...@lists.windowmaker.org.

Re: [PATCH] wmaker: src/action.c merge duplicate code

[Rodolfo García Peñas (kix)]

"Rodolfo García Peñas (kix)"  escribió:


David Maciejak  escribió:


No problem i understand, enclosed the new patch.


Hi David,

is possible remove the extra curly brackets (in both "if"s)?:


Hi again,

Perhaps we can change this "if" from the negative way to possitive way:

+   if (!wwin->screen_ptr->flags.startup && !wPreferences.no_animations
+   && !wwin->flags.skip_next_animation && wwin->icon != NULL) {

to

+   if (wwin->screen_ptr->flags.startup || wPreferences.no_animations ||
+   wwin->flags.skip_next_animation || wwin->icon == NULL) {

Then we can return earlier:

+   if (wwin->screen_ptr->flags.startup || wPreferences.no_animations ||
+   wwin->flags.skip_next_animation || wwin->icon == NULL)
+   return 0;

And then add the rest of the code with less tabs:

+   if (!wPreferences.disable_miniwindows
+   && !wwin->flags.net_handle_icon) {
+   *ix = wwin->icon_x;
+   *iy = wwin->icon_y;
+   *iw = wwin->icon->core->width;
+   *ih = wwin->icon->core->height;
+   } else {
+   if (wwin->flags.net_handle_icon) {
+   *ix = wwin->icon_x;
+   *iy = wwin->icon_y;
+   *iw = wwin->icon_w;
+   *ih = wwin->icon_h;
+   } else {
+   *ix = 0;
+   *iy = 0;
+   *iw = wwin->screen_ptr->scr_width;
+   *ih = wwin->screen_ptr->scr_height;
+   }
+   }
+   return 1;
+}

What do you think? Perhaps could be an idea to add in a different
patch, following the Carlos recommendation about git bisect,...

Cheers,
kix.
Rodolfo García Peñas (kix)
http://www.kix.es/


--
To unsubscribe, send mail to wmaker-dev-unsubscr...@lists.windowmaker.org.