Opened 8 years ago

Closed 8 years ago

#7052 closed enhancement (fixed)

Deskbar Style Changes and Refactoring

Reported by: jscipione Owned by: tqh
Priority: normal Milestone: R1
Component: Applications/Deskbar Version: R1/Development
Keywords: style refactoring Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

This might be a small abuse of the trac system but I created this ticket so I'd have a place to put patches that relate to style changes and refactoring. None of these changes should have any effect on Deskbar from a user's perspective but they either make the code style nicer, or eliminate code duplication, or give a variable a better name.

Attachments (50)

Deskbar Style Changes and Refactoring.diff (8.8 KB) - added by jscipione 8 years ago.
Please review this patch, if you approve the changes, apply the patch and close this ticket, if you don't tell me what is wrong. Thanks
Deskbar WindowMenu Refactoring 1.diff (2.4 KB) - added by jscipione 8 years ago.
Does some refactoring on WindowMenuBar.cpp and WindowMenuBar.h that I noticed while fixing more things
Deskbar Style Changes and Refactoring 2.diff (8.2 KB) - added by jscipione 8 years ago.
Call BarView() function to get the barView pointer instead of creating an fBarView pointer
Deskbar Style Changes and Refactoring 3.diff (71.1 KB) - added by jscipione 8 years ago.
Deskbar Style Super Mega Patch. Delete evil spaces at the beginning and end of lines. No your comments are not so special that they deserve and extra indent. No your comments are not so special that they get a tab after instead of a space. No you cannot use a tab instead of a space if the tab happens to indent by 1 space. Spaces around binary operators. Other stuff.
Deskbar Style Changes and Refactoring 4.diff (123.1 KB) - added by jscipione 8 years ago.
This patch is big vim style checker found even more style issues including that the Open Tracker license broke the 80 char limit in almost every file! yeah! Also more evil spaces deleted, more 80 char limit fixes, more spaces around binary operators. Single line comments get indented *after* the line they affect as per style guide.
Deskbar Autoexpand Teams 1.diff (8.8 KB) - added by jscipione 8 years ago.
Deskbar Disable Expand Options 1.diff (3.0 KB) - added by jscipione 8 years ago.
Deskbar Resize Team Icons 5.diff (20.3 KB) - added by jscipione 8 years ago.
Deskbar_BarApp_cpp_style_fix.diff (7.2 KB) - added by jscipione 8 years ago.
Deskbar_BarApp_h_style_fix.diff (494 bytes) - added by jscipione 8 years ago.
Deskbar_BarMenuBar_cpp_style_fix.diff (2.8 KB) - added by jscipione 8 years ago.
Deskbar_BarMenuBar_h_style_fix.diff (1.5 KB) - added by jscipione 8 years ago.
Deskbar_BarMenuTitle_cpp_style_fix.diff (1.7 KB) - added by jscipione 8 years ago.
Deskbar_BarMenuTitle_h_style_fix.diff (1.6 KB) - added by jscipione 8 years ago.
Deskbar_BarView_cpp_style_fix.diff (7.5 KB) - added by jscipione 8 years ago.
Deskbar_BarView_h_style_fix.diff (1.3 KB) - added by jscipione 8 years ago.
Deskbar_BarWindow_cpp_style_fix.diff (3.6 KB) - added by jscipione 8 years ago.
Deskbar_BarWindow_h_style_fix.diff (955 bytes) - added by jscipione 8 years ago.
Deskbar_BeMenu_cpp_style_fix.diff (1.2 KB) - added by jscipione 8 years ago.
Deskbar_BarWindow_h_style_fix.2.diff (955 bytes) - added by jscipione 8 years ago.
Deskbar_CalendarMenuWindow_cpp_style_fix.diff (567 bytes) - added by jscipione 8 years ago.
Deskbar_BeMenu_h_style_fix.diff (2.0 KB) - added by jscipione 8 years ago.
Deskbar_CalendarMenuWindow_h_style_fix.diff (802 bytes) - added by jscipione 8 years ago.
Deskbar_DeskBarUtils_cpp_style_fix.diff (2.1 KB) - added by jscipione 8 years ago.
Deskbar_DeskBarUtils_h_style_fix.diff (947 bytes) - added by jscipione 8 years ago.
Deskbar_ExpandoMenuBar_cpp_style_fix.diff (3.9 KB) - added by jscipione 8 years ago.
Deskbar_ExpandoMenuBar_h_style_fix.diff (1.2 KB) - added by jscipione 8 years ago.
Deskbar_LICENSE_style_fix.diff (912 bytes) - added by jscipione 8 years ago.
Deskbar_PreferencesWindow_cpp_style_fix.diff (1.6 KB) - added by jscipione 8 years ago.
Deskbar_PreferencesWindow_h_style_fix.diff (549 bytes) - added by jscipione 8 years ago.
Deskbar_ResourceSet_cpp_style_fix.diff (13.7 KB) - added by jscipione 8 years ago.
Deskbar_ResourceSet_h_style_fix.diff (2.9 KB) - added by jscipione 8 years ago.
Deskbar_ShowHideMenuItem_cpp_style_fix.diff (971 bytes) - added by jscipione 8 years ago.
Deskbar_ShowHideMenuItem_h_style_fix.diff (1.7 KB) - added by jscipione 8 years ago.
Deskbar_StatusView_cpp_style_fix.diff (10.3 KB) - added by jscipione 8 years ago.
Deskbar_StatusView_h_style_fix.diff (1.9 KB) - added by jscipione 8 years ago.
Deskbar_StatusViewShelf_cpp_style_fix.diff (1.7 KB) - added by jscipione 8 years ago.
Deskbar_StatusViewShelf_h_style_fix.diff (1.7 KB) - added by jscipione 8 years ago.
Deskbar_Switcher_cpp_style_fix.diff (16.2 KB) - added by jscipione 8 years ago.
Deskbar_Switcher_h_style_fix.diff (1.4 KB) - added by jscipione 8 years ago.
Deskbar_TeamMenu_cpp_style_fix.diff (1.2 KB) - added by jscipione 8 years ago.
Deskbar_TeamMenu_h_style_fix.diff (1.3 KB) - added by jscipione 8 years ago.
Deskbar_TeamMenuItem_cpp_style_fix.diff (8.5 KB) - added by jscipione 8 years ago.
Deskbar_TeamMenuItem_h_style_fix.diff (1.6 KB) - added by jscipione 8 years ago.
Deskbar_TimeView_cpp_style_fix.diff (2.5 KB) - added by jscipione 8 years ago.
Deskbar_TimeView_h_style_fix.diff (1.4 KB) - added by jscipione 8 years ago.
Deskbar_WindowMenu_cpp_style_fix.diff (4.3 KB) - added by jscipione 8 years ago.
Deskbar_WindowMenu_h_style_fix.diff (1.2 KB) - added by jscipione 8 years ago.
Deskbar_WindowMenuItem_cpp_style_fix.diff (2.5 KB) - added by jscipione 8 years ago.
Deskbar_WindowMenuItem_h_style_fix.diff (1.3 KB) - added by jscipione 8 years ago.

Download all attachments as: .zip

Change History (67)

Changed 8 years ago by jscipione

Please review this patch, if you approve the changes, apply the patch and close this ticket, if you don't tell me what is wrong. Thanks

comment:1 Changed 8 years ago by jscipione

Has a Patch: set

Changed 8 years ago by jscipione

Does some refactoring on WindowMenuBar.cpp and WindowMenuBar.h that I noticed while fixing more things

comment:2 Changed 8 years ago by tqh

Owner: changed from axeld to tqh
Status: newassigned

I guess I'll try to get my hands dirty with a style review tomorrow. This will probably end well ;)

comment:3 Changed 8 years ago by tqh

While I think your changes looks ok from a quick view, I find there is no need for the silliness done in TBarApp using be_app to call BarView(). I can't really see why TBarApp can't call BarView() directly. If I'm not mistaken I think that call would be better than introducing a member for fBarView.

Why use this source:haiku/trunk/src/apps/deskbar/BarApp.cpp#L426 when this source:haiku/trunk/src/apps/deskbar/BarApp.h#L135 can be called directly?

comment:4 Changed 8 years ago by jscipione

Thank you for reviewing my patch. I grabbed a pointer to the barView in the constructor because the code to grab the barView was being called many times. This way, the pointer to the barView is gotten just once, and is used several times rather than having to grab the pointer over and over again. DRY

comment:5 Changed 8 years ago by jscipione

I see what you mean about calling BarView() directly though, I didn't see that function, apparently neither did the original author. I can rework the patch to use that function instead.

comment:6 Changed 8 years ago by tqh

A suggestion from DeadYak:

looking at the context it's called in, I'd honestly say it might be better to update the settings and then resend the message to BarWindow to process

So you might want to try and see if the old code can't be written even better. You can ofc just stick to style improvement if you don't want to work on that. Any work is appreciated.

comment:7 Changed 8 years ago by jscipione

I created this particular ticket strictly for style changes only. Any functional changes would go somewhere else. Since I really want the other style changes in the above patches committed (gets em' out of my patches) I'll just remove the controversial bits for now.

Changed 8 years ago by jscipione

Call BarView() function to get the barView pointer instead of creating an fBarView pointer

Changed 8 years ago by jscipione

Deskbar Style Super Mega Patch. Delete evil spaces at the beginning and end of lines. No your comments are not so special that they deserve and extra indent. No your comments are not so special that they get a tab after instead of a space. No you cannot use a tab instead of a space if the tab happens to indent by 1 space. Spaces around binary operators. Other stuff.

Changed 8 years ago by jscipione

This patch is big vim style checker found even more style issues including that the Open Tracker license broke the 80 char limit in almost every file! yeah! Also more evil spaces deleted, more 80 char limit fixes, more spaces around binary operators. Single line comments get indented *after* the line they affect as per style guide.

comment:8 Changed 8 years ago by tqh

Nice. The patches is beginning to be large, so further changes should probably be divided in to several patches. This way one patch at a time can be reviewed and applied easily.

There is a branch for Deskbar/Tracker rewrite that might be affected if to big changes are done. I do not know what the state of that branch is, so I'm a bit worried of how to handle this. It probably need more devs though, as you have seen the code can be improved and refactored.

comment:9 in reply to:  8 Changed 8 years ago by aldeck

Replying to tqh:

There is a branch for Deskbar/Tracker rewrite that might be affected if to big changes are done. I do not know what the state of that branch is, so I'm a bit worried of how to handle this. It probably need more devs though, as you have seen the code can be improved and refactored.

Don't worry, the tracker_layout branch only touches src/kits/Tracker. I'm taking a Haiku break at the moment but i'm still around :)

comment:10 Changed 8 years ago by jscipione

The patches is beginning to be large, so further changes should probably be divided in to several patches. This way one patch at a time can be reviewed and applied easily.

Yes it is large, there are a lot of style problems :) The only style problems I see that are controversial are (line numbers from Deskbar Style Changes and Refactoring 4.diff)

    StatusView.cpp line 1044
    Switcher.cpp line 1470
    Switcher.cpp line 2066

Those comments maybe should be indented. Perhaps I got a little bit too zealous with the unindenting. Perhaps some style expert can comment here.

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

Changed 8 years ago by jscipione

comment:11 Changed 8 years ago by jscipione

I really wish there was a way to upload multiple patches at once... anyway I split my patch into individual diff files so they can be reviewed and applied separately. Because they are style changes _only_ you should be able to apply each file separately without breaking anything. I also accidentally uploaded some unrelated patches so please ignore Deskbar Autoexpand Teams 1.diff, Deskbar Disable Expand Options 1.diff, and Deskbar Resize Team Icons 5.diff. Those are patches to other things, not style related. I marked them as obsolete to reduce the confusion. Sorry I have way too many patches floating around. Anyway, if you like patches you'll love this ticket. Please review my patches and apply them if they are good, fix them if they are not. Thanks!

comment:12 Changed 8 years ago by tqh

  • Deskbar_ExpandoMenuBar_cpp_style_fix.diff
  • Deskbar_ExpandoMenuBar_h_style_fix.diff
  • Deskbar_LICENSE_style_fix.diff

applied in hrev40744.

comment:13 Changed 8 years ago by tqh

Deskbar_BarApp patches applied in hrev40814.

comment:14 Changed 8 years ago by tqh

  • Deskbar_BarMenuBar_cpp_style_fix.diff
  • Deskbar_BarMenuBar_h_style_fix.diff
  • Deskbar_BarMenuTitle_cpp_style_fix.diff
  • Deskbar_BarMenuTitle_h_style_fix.diff
  • Deskbar_BarView_cpp_style_fix.diff
  • Deskbar_BarView_h_style_fix.diff

Applied in hrev40815.

comment:15 Changed 8 years ago by tqh

  • Deskbar_BarWindow_cpp_style_fix.diff
  • Deskbar_BarWindow_h_style_fix.diff
  • Deskbar_BeMenu_cpp_style_fix.diff
  • Deskbar_CalendarMenuWindow_cpp_style_fix.diff
  • Deskbar_BeMenu_h_style_fix.diff
  • Deskbar_CalendarMenuWindow_h_style_fix.diff
  • Deskbar_DeskBarUtils_cpp_style_fix.diff
  • Deskbar_DeskBarUtils_h_style_fix.diff

Applied in hrev40817 and hrev40818.

comment:16 Changed 8 years ago by tqh

  • Deskbar_PreferencesWindow_cpp_style_fix.diff
  • Deskbar_PreferencesWindow_h_style_fix.diff
  • Deskbar_ResourceSet_cpp_style_fix.diff
  • Deskbar_ResourceSet_h_style_fix.diff
  • Deskbar_ShowHideMenuItem_cpp_style_fix.diff
  • Deskbar_ShowHideMenuItem_h_style_fix.diff
  • Deskbar_StatusView_cpp_style_fix.diff
  • Deskbar_StatusView_h_style_fix.diff
  • Deskbar_StatusViewShelf_cpp_style_fix.diff
  • Deskbar_StatusViewShelf_h_style_fix.diff

Applied in hrev40847.

comment:17 Changed 8 years ago by tqh

Resolution: fixed
Status: assignedclosed
  • Deskbar_Switcher_cpp_style_fix.diff
  • Deskbar_Switcher_h_style_fix.diff
  • Deskbar_TeamMenu_cpp_style_fix.diff
  • Deskbar_TeamMenu_h_style_fix.diff
  • Deskbar_TeamMenuItem_cpp_style_fix.diff
  • Deskbar_TeamMenuItem_h_style_fix.diff
  • Deskbar_TimeView_cpp_style_fix.diff
  • Deskbar_TimeView_h_style_fix.diff
  • Deskbar_WindowMenu_cpp_style_fix.diff
  • Deskbar_WindowMenu_h_style_fix.diff
  • Deskbar_WindowMenuItem_cpp_style_fix.diff
  • Deskbar_WindowMenuItem_h_style_fix.diff

Applied in hrev40849. Hopefully I didn't miss anything.

Note: See TracTickets for help on using tickets.