GNOME Bugzilla – Bug 780490
Add an option to wrap over workspace boundaries on WnckPager widget
Last modified: 2017-04-26 07:31:14 UTC
This enhancement adds optional functionality to enable a kind of circular workspace switching for the WnckPager widget.
Created attachment 348629 [details] [review] Add wrap-on-scroll option for WnckPager widget This commit is a minor and optional enhancement of the WnckPager widget. It allows to scroll over the borders of workspace boundaries. This behavior, which is disabled by default, might be enabled with wnck_pager_set_wrap_on_scroll. When activated, a scroll event which would otherwise do nothing, i.e. over the border, will now scroll from the first to the last workspace and from the last to the first. With this option disabled the behavior is exactly the same as before. The minor version number has been increased to signal an API addition.
Review of attachment 348629 [details] [review]: I am not libwnck maintainer, but... ::: configure.ac @@ +3,2 @@ m4_define([wnck_major_version], [3]) +m4_define([wnck_minor_version], [21]) I think that patch should not change version. Odd numbers are used to indicate development version, stable version have even numbers. ::: libwnck/pager.c @@ +2080,3 @@ case GDK_SCROLL_DOWN: if (index + n_columns < n_workspaces) + { Does not much style used in libwnck. Two spaces before { and }. @@ +2085,3 @@ + else if (wrap_workspaces && index == n_workspaces - 1) + { + /* reached the last workspace, wrap arround to first */ Personally I think such comments should not be added. @@ +2444,3 @@ +void +wnck_pager_set_wrap_on_scroll (WnckPager * pager, + gboolean wrap_on_scroll) Wrong indentation / style.
Review of attachment 348629 [details] [review]: ::: libwnck/pager.c @@ +2434,3 @@ + * wnck_pager_set_wrap_on_scroll: + * @pager: a #WnckPager. + * @wrap_on_scroll: a shadow type. shadow type?
(In reply to Alberts Muktupāvels from comment #2) > Review of attachment 348629 [details] [review] [review]: > > I am not libwnck maintainer, but... > > ::: configure.ac > @@ +3,2 @@ > m4_define([wnck_major_version], [3]) > +m4_define([wnck_minor_version], [21]) > > I think that patch should not change version. Odd numbers are used to > indicate development version, stable version have even numbers. It will have to be a new version. Otherwise there is no way to depend on the new API additions. I'm not familiar with how that is usually handled. I'm open for other suggestions (and of course I can make that number even). > > ::: libwnck/pager.c > @@ +2080,3 @@ > case GDK_SCROLL_DOWN: > if (index + n_columns < n_workspaces) > + { > > Does not much style used in libwnck. Two spaces before { and }. Didn't notice that, thanks. > > @@ +2085,3 @@ > + else if (wrap_workspaces && index == n_workspaces - 1) > + { > + /* reached the last workspace, wrap arround to first */ > > Personally I think such comments should not be added. What's your rationale? To be honest I generally miss comments in a lot of open source projects. Simply because the code is in many cases not self-explanatory. (Nevertheless I will do as requested.) > > @@ +2444,3 @@ > +void > +wnck_pager_set_wrap_on_scroll (WnckPager * pager, > + gboolean wrap_on_scroll) > > Wrong indentation / style. What's the prefered style? Is there some style guide I could look at? I'm also confused with mixed tab and whitespace identation. > shadow type? Copy-paste error. Fixed. Creating a new patch when everything is addressed.
(In reply to muesli4 from comment #4) > (In reply to Alberts Muktupāvels from comment #2) > > Review of attachment 348629 [details] [review] [review] [review]: > > > > I am not libwnck maintainer, but... > > > > ::: configure.ac > > @@ +3,2 @@ > > m4_define([wnck_major_version], [3]) > > +m4_define([wnck_minor_version], [21]) > > > > I think that patch should not change version. Odd numbers are used to > > indicate development version, stable version have even numbers. > It will have to be a new version. Otherwise there is no way to depend on the > new API additions. I'm not familiar with how that is usually handled. I'm > open for other suggestions (and of course I can make that number even). Last libwnck version is 3.20.1, but master branch has 3.20.2. So if this is merged in master then you can depend on 3.20.2 and it should work. And it does not matter if it will be released as 3.20.2 or 3.22.0. This is my opinion, again, I am not libwnck maintainer... > > > > ::: libwnck/pager.c > > @@ +2080,3 @@ > > case GDK_SCROLL_DOWN: > > if (index + n_columns < n_workspaces) > > + { > > > > Does not much style used in libwnck. Two spaces before { and }. > Didn't notice that, thanks. > > > > > @@ +2085,3 @@ > > + else if (wrap_workspaces && index == n_workspaces - 1) > > + { > > + /* reached the last workspace, wrap arround to first */ > > > > Personally I think such comments should not be added. > What's your rationale? To be honest I generally miss comments in a lot of > open source projects. Simply because the code is in many cases not > self-explanatory. (Nevertheless I will do as requested.) It is very simple if statement. How code will look if every simple thing is commented? It is not request, it is my opinion. Please wait maintainer comment / review. > > @@ +2444,3 @@ > > +void > > +wnck_pager_set_wrap_on_scroll (WnckPager * pager, > > + gboolean wrap_on_scroll) > > > > Wrong indentation / style. > What's the prefered style? Is there some style guide I could look at? I'm > also confused with mixed tab and whitespace identation. Check one or more functions above and it should be clear how it should be formatted. Don't mix tabs and spaces, just use spaces. > > shadow type? > Copy-paste error. Fixed. > > Creating a new patch when everything is addressed.
Review of attachment 348629 [details] [review]: Please address the comments that Alberts wrote, I agree with those. I also think you should add a wnck_pager_get_wrap_on_scroll function too at this point. ::: configure.ac @@ +3,2 @@ m4_define([wnck_major_version], [3]) +m4_define([wnck_minor_version], [21]) Yeah, agree... Please don't change this. ::: libwnck/pager.c @@ +2098,3 @@ if (index < n_workspaces - 1) index++; + else if (wrap_workspaces) Also add brackets around index at this point (here and above - when reaching the first one).
Created attachment 350314 [details] [review] The new patch with all issues addressed. Thanks so much for reviewing. I hope I addressed all your concerns.
Review of attachment 350314 [details] [review]: ::: libwnck/pager.c @@ +2451,3 @@ + */ +void +wnck_pager_set_wrap_on_scroll (WnckPager *pager, reduce space between WnckPager and *pager. in this case there should be only one space. @@ +2482,3 @@ } + new unneeded line.
Created attachment 350327 [details] [review] Hopefully the final patch. Fixed issues and added an option to test-pager.
Review of attachment 350327 [details] [review]: ::: libwnck/pager.c @@ +2452,3 @@ +void +wnck_pager_set_wrap_on_scroll (WnckPager *pager, + gboolean wrap_on_scroll) parameters should be aligned. @@ +2468,3 @@ + * Since: 3.20.2 + */ +gboolean wnck_pager_get_wrap_on_scroll (WnckPager *pager) - new line - one space - g_return_val_if_fail
Created attachment 350336 [details] [review] The patch.
Review of attachment 350336 [details] [review]: ::: libwnck/pager.c @@ +2452,3 @@ +void +wnck_pager_set_wrap_on_scroll (WnckPager *pager, + gboolean wrap_on_scroll) still not aligned @@ +2469,3 @@ + */ +gboolean +wnck_pager_get_wrap_on_scroll (WnckPager *pager) space ::: libwnck/pager.h @@ +95,3 @@ void wnck_pager_set_shadow_type (WnckPager *pager, GtkShadowType shadow_type); +void wnck_pager_set_wrap_on_scroll (WnckPager *pager, oh, I guess here extra space is not needed - between `*_on_scroll` and `(`
(In reply to Alberts Muktupāvels from comment #12) > @@ +2469,3 @@ > + */ > +gboolean > +wnck_pager_get_wrap_on_scroll (WnckPager *pager) > > space Could you direct me to a style guide? Because at many other places there are extra spaces and I am getting confused about where to put space or not. > > ::: libwnck/pager.h > @@ +95,3 @@ > void wnck_pager_set_shadow_type (WnckPager *pager, > GtkShadowType shadow_type); > +void wnck_pager_set_wrap_on_scroll (WnckPager *pager, > > oh, I guess here extra space is not needed - between `*_on_scroll` and `(` My assumption was that parameters in the header file line up on the names which I tried to achieve.
(In reply to muesli4 from comment #13) > (In reply to Alberts Muktupāvels from comment #12) > > @@ +2469,3 @@ > > + */ > > +gboolean > > +wnck_pager_get_wrap_on_scroll (WnckPager *pager) > > > > space > > Could you direct me to a style guide? Because at many other places there are > extra spaces and I am getting confused about where to put space or not. parameters should be aligned. In this function there is only one parameter and it does not require extra space to align parameters. So you need only one space between WnckPager and *pager. No idea about style guide... > > > > ::: libwnck/pager.h > > @@ +95,3 @@ > > void wnck_pager_set_shadow_type (WnckPager *pager, > > GtkShadowType shadow_type); > > +void wnck_pager_set_wrap_on_scroll (WnckPager *pager, > > > > oh, I guess here extra space is not needed - between `*_on_scroll` and `(` > > My assumption was that parameters in the header file line up on the names > which I tried to achieve. Check above functions. Simply wnck_pager_set_wrap_on_scroll is too large in this case. Just remove extra space `*_on_scroll` and `(` so there is only one space and make sure parameters are still aligned and then it should be good.
Created attachment 350351 [details] [review] The patch. As I could not make out any single consistent style but rather a lot of mixed ones, I used the following style guideline as a basis: https://developer.gnome.org/programming-guidelines/stable/c-coding-style.html.en In that manner I restructured the header file (as I would have to do that anyway, since my added declaration changed the identation of all parameter names).
Review of attachment 350351 [details] [review]: Looks good to me.
Review of attachment 350351 [details] [review]: Looks good now, thanks.