Created attachment 339877 [details] [review]
Cleaning baseview.py

As for release 3.24 it is required to clean the codebase
according to PEP8 and PEP257.

CleanUp baseview.py along with some related files

Hi, this is not the final attachment. There are many things remaining but i had some doubts. Thnks in advance :)

## Regarding line 164 in the patch
track_playing = self.player.currentTrack is not None

## Option

track_playing = False
if(self.player.currentTrack):
    track_playing = True

## i am not sure whether that is suitable. Please suggest. Thanks :)

## Also, regarding line 251,
self.cache.lookup(item, self._iconWidth, self._iconHeight,
                  self._on_lookup_ready, itr)

## _iconWidth and _iconHeight are not used anywhere in my under-
## standing except this line. What should be done here?

Review of attachment 339877 [details] [review]:

You're on the right track here.

::: gnomemusic/views/baseview.py
@@ +35,2 @@
 class BaseView(Gtk.Stack):
+    """Base Class for all view classes"""

Base class for all views

@@ +61,3 @@
         self._adjustmentValueId = 0
         self._adjustmentChangedId = 0
         self._scrollbarVisibleId = 0

These need to be unCamelCased, are they even used?

@@ +99,1 @@
         self.window = window

These things are really internal I think, so self._window . Inherated classes can do just the same if initialized properly.

Note that this probably also requires changes in base classes.

@@ +136,3 @@
 
+        self.view.click_handler = self.view.connect('item-activated',
+            self._on_item_activated)

If not next-lining from the opening parenthesis it should be aligning with the parenthesis.

Eg.
self.v.connect(arg1,
               arg2)

vs

self.v.connect(
    arg1, arg2)

What the best choice is really depends on how long the statement is and the size of the arguments.

(this doesn't line out correctly in the browser, but I hope you get the idea)

@@ +159,3 @@
+                .set_sensitive(False)
+            self.selection_toolbar._remove_from_playlist_button\
+                .set_sensitive(False)

Note that this is bad (although not your problem) : we are accessing private member of selection_toolbar (_add_to*, _remove_from*).

Other than that I dislike the '\' line break : see how you have 3 lines of self.selection_toolbar, maybe make a tmp var for it (eg. toolbar = self.selection_toolbar) and now see if you can fit the following lines.

@@ +162,3 @@
         else:
             self.header_bar.set_selection_mode(False)
+            track_playing = self.player.currentTrack is not None

Regarding your question in the comment: this is fine. currenTrack can be set to None (I guess if nothing is playing) and so this is a valid test. Your suggestion in the comment does the same but is more complicated (more lines).

@@ +202,3 @@
             self.header_bar._selection_menu_label.set_text(
+                ngettext("Selected %d item", "Selected %d items", n_items)
+                         % n_items)

I want to start make use of .format() for string formatting, maybe you can convert this as well. There are some examples in the code already.
Other than that you can split the statements up if the lines are getting too long.

@@ +205,3 @@
         else:
+            self.header_bar._selection_menu_label.set_text(_(
+                "Click on items to select them"))

This is technically ok, but I would definitely break directly after the first opening parenthesis.

@@ +224,3 @@
         self.window.notification.set_timeout(0)
         if not item:
+            if not remaining:

I think the original statement is better, remaining is not a bool and the not operator is a boolean operator. Check for the integer value.

@@ +250,3 @@
         ]
         self.cache.lookup(item, self._iconWidth, self._iconHeight,
+                          self._on_lookup_ready, itr)

Regarding your comment. This is an oversight, this lookup line doesn't work anymore and should be converted to use ArtSize (check albumartcache).

However, since this is in live code and I've never seen a problem I highly doubt this bit of code is ever reached, so maybe its time to remove it (we might need to keep the function definition for inheritance).

@@ +280,3 @@
         count = 0
+        itr = self.model.iter_children(parent)
+        while itr:

Ok, this is really nitpicky, but I think checking for None is better. itr is either an object or None, if you check this implicitly the object has to be converted to bool all the time.

I must say, I might not be very consistent with this myself.

@@ +283,3 @@
+            if self.model.iter_has_child(itr):
+                count += self._set_selection(value, itr)
+            if self.model[itr][5]:

<nitpicking mode> is the fifth element is not a boolean object then it should be checking for None.

::: gnomemusic/views/playlistview.py
@@ +253,2 @@
         self.model.set_value(currentIter, 10, True)
+        if self.model.get_value(currentIter, 8) != self.error_icon_name:

as long as you are changing the line anyway, shorthands:
self.model[currentIter][8]

Created attachment 340290 [details] [review]
Cleaning baseview.py

As for release 3.24 it is required to clean the codebase
according to PEP8 and PEP257.

CleanUp baseview.py along with some related files.

Hi, thanks for the great help. Please suggest other improvements.

Another doubt regarding calling private members at various 
places in baseview.py. Could we make them non private or
should i try to write wrappers which in place could call those
private members. Please suggest. Thanks :)

Review of attachment 340290 [details] [review]:

Pretty good, some minor comments.

However, one big thing is that the patch changes unrelated things outside of baseview. I think those should be left for the respective cleanups of those files. Otherwise patches randomly touch stuff all over the place. So that needs to be reverted for this patch.

::: gnomemusic/views/baseview.py
@@ +195,3 @@
             self.header_bar._selection_menu_label.set_text(
+                ngettext("Selected {} item", "Selected {} items", n_items)\
+                         .format(n_items))

I dislike the \ so much that i prefer you just put the ngettext items on seperate lines and add the .format after closing parenthesis.

Another way to get more space is making a shortname var of self.header_bar._selection_menu_label .

@@ +301,3 @@
+        select_toolbar._remove_from_playlist_button.set_sensitive(False)
+        self.header_bar._selection_menu_label\
+            .set_text(_("Click on items to select them"))

Get rid of the \

preferable would be .set_text on the first line and the argument (=gettext (_) call) on the next.

::: gnomemusic/views/playlistview.py
@@ +203,3 @@
             return
 
+        item = model[_iter][5]

although this is the correct cleanup, it has nothing todo with baseview. I think we should confine these cleanups to just the file it is about.

@@ +230,3 @@
 
+        if model[_iter][11] == DiscoveryStatus.FAILED:
+            cell.set_property('icon-name', self.error_icon_name)

this however is fine: self.error_icon_name is defined in baseview

Created attachment 340371 [details] [review]
Cleaning baseview.py

As for release 3.24 it is required to clean the codebase
according to PEP8 and PEP257.

CleanUp baseview.py along with some related files.

Review of attachment 340371 [details] [review]:

Pretty much done I guess, but any classwide that shouldn't be used outside should be marked as such.

::: gnomemusic/views/baseview.py
@@ +38,2 @@
+    now_playing_icon_name = 'media-playback-start-symbolic'
+    error_icon_name = 'dialog-error-symbolic'

These are internal.

@@ +97,3 @@
         self.header_bar = window.toolbar
         self.selection_toolbar = window.selection_toolbar
         self.header_bar._select_button.connect(

Actually all this should be internal I guess.

@@ +108,2 @@
         self.show_all()
         self.view.hide()

view internal

(In reply to Suyash Garg from comment #5)
> Hi, thanks for the great help. Please suggest other improvements.
> 
> Another doubt regarding calling private members at various 
> places in baseview.py. Could we make them non private or
> should i try to write wrappers which in place could call those
> private members. Please suggest. Thanks :)

Eventually we should do something with it, but right now this is just about cleaning up. However at some point we should look at what is needed to get proper access to these objects/variables if any is needed. Consider that phase 2.

Created attachment 340395 [details] [review]
Cleaning baseview.py

As for release 3.24 it is required to clean the codebase
according to PEP8 and PEP257.

CleanUp baseview.py along with some related files.

Created attachment 346132 [details] [review]
Cleaning baseview.py

As for release 3.24 it is required to clean the codebase
according to PEP8 and PEP257.

CleanUp baseview.py along with some related files.

Sorry for the delay, this sort of slipped away. I rebased it for current master.

Created attachment 346257 [details] [review]
Cleaning baseview.py

As for release 3.24 it is required to clean the codebase
according to PEP8 and PEP257.

CleanUp baseview.py along with some related files.

Created attachment 346579 [details] [review]
baseview: Cleanup

Make the code follow PEP-8 & PEP-257 recommendations, this work also
includes derived classes for as much as is needed.

Made some more minor modifications and pushed.

Thanks for the patch.
Attachment 346579 [details] pushed as 4c58af4 - baseview: Cleanup