Created attachment 341716 [details] [review]
searchview.py: Clean up code and format to PEP8

Comment on attachment 341716 [details] [review]
searchview.py: Clean up code and format to PEP8

I haven't completed doing it. There is lots of work still let.

Review of attachment 341716 [details] [review]:

Just searchview. The point is to not go all over the place and change every file. Anything that is defined in searchview is up for cleaning.

Good effort, but once again you are trying to do too much. This is not about rewriting stuff.

If you got a good rewrite of a part of the code that's fine, but it shouldn't be part of the same commit.

::: gnomemusic/utils.py
@@ +104,3 @@
+        return 2
+
+

This is unneeded, a workaround for something that should be dealt with in a proper way.

::: gnomemusic/views/searchview.py
@@ +70,1 @@
         self.albums_selected = []

this should probably be _ prepended

@@ +87,3 @@
         self.window._stack.set_visible_child_name('emptysearch')
         emptysearch = self.window._stack.get_child_by_name('emptysearch')
+        emptysearch._artist_albums_widget = self._artist_albums_widget

That's not defined in this file/class

@@ +130,3 @@
                 artist, albums, self.player,
                 self.header_bar, self.selection_toolbar, self.window, True
             )

This should be on 2 lines and the closing parenthesis on the same line,

@@ +154,3 @@
     def _on_selection_mode_changed(self, widget, data=None):
+        if (self._artist_albums_widget is not None and
+                self.get_visible_child() == self._artist_albums_widget):

and prepended on the 2nd line, see other pep8-ed files

@@ +156,3 @@
+                self.get_visible_child() == self._artist_albums_widget):
+            self._artist_albums_widget.set_selection_mode(self.header_bar.
+                                                          _selectionMode)

Don't indent on a dot, just put out on the next line.

@@ +204,3 @@
+        if (category == 'song' and
+                self.found_items_number == 0 and
+                remaining == 0):

like the other case
if (this
  and that
  and that)

@@ +226,3 @@
         artist = utils.get_artist_name(item)
         composer = utils.get_composer_name(item)
+        favourite = utils.get_favourite(item, source, category)

don't try to add things that do not belong in cleanup

@@ +237,3 @@
         # icon for the search view.
         _iter = None
+        if category == 'artist' and artist.casefold() in self._artists:

i have no idea whats going on here.. you removed a bunch of stuff?

It doesn't look to do quite the same thing to me.

This is mainly a cleanup, removal of unused stuff, etc. Not a rewrite.

@@ +287,3 @@
+            cell.set_visible(self.view.get_selection_mode())
+        else:
+            cell.set_visible(False)

this is much more readable.

Created attachment 341882 [details] [review]
searchview.py: Clean up code and format to PEP-8

*** Bug 776879 has been marked as a duplicate of this bug. ***

Created attachment 342906 [details] [review]
searchview.py: Change according to PEP8

Fixes https://bugzilla.gnome.org/show_bug.cgi?id=775926

https://bugzilla.gnome.org/show_bug.cgi?id=776879

The patch is awaiting review :)

Review of attachment 342906 [details] [review]:

Your indentation deviates a lot with what we do at other places, look at the other cleanup commits to see what where to go there. The main idea is too keep it readable and logical, eg. (key1, key2[3]), indenting on the [ makes it pretty much unreadable, key2[3] is one argument.

So if you can fix this, I guess we can go from there.

::: gnomemusic/views/searchview.py
@@ +144,3 @@
             self.header_bar.searchbar.show_bar(False)
         elif self.model[_iter][11] == 'song':
             if self.model.get_value(_iter, 12) != DiscoveryStatus.FAILED:

self.model[_iter][12]

@@ +146,3 @@
             if self.model.get_value(_iter, 12) != DiscoveryStatus.FAILED:
+                child_iter = self.songs_model.convert_child_iter_to_iter(_iter)[
+                                                                         1]

I don't like this indentation. Maybe make child_iter a shorter var, you only use it once.

@@ +148,3 @@
+                                                                         1]
+                self.player.set_playlist(
+                    'Search Results', None, self.songs_model, child_iter, 5, 12)

I don't like this indentation, try as much as possible on the first line and the next line by the (.

@@ +158,3 @@
     @log
     def _on_selection_mode_changed(self, widget, data=None):
         if self._artistAlbumsWidget is not None and self.get_visible_child() == self._artistAlbumsWidget:

Line is too long. Look at other cleanups.

if (<arg 1>
       and <arg 2>):

@@ +160,3 @@
         if self._artistAlbumsWidget is not None and self.get_visible_child() == self._artistAlbumsWidget:
+            self._artistAlbumsWidget.set_selection_mode(
+                self.header_bar._selectionMode)

If this line isn't too long on 1 line.

@@ +185,3 @@
             self._albums[key].tracks = []
+            self._add_item(source, None, self._albums[
+                           key], 0, [self.model, 'album'])

don't indent on [ here, just the commas should do

@@ +187,3 @@
+                           key], 0, [self.model, 'album'])
+            self._add_item(source, None, self._albums[
+                           key], 0, [self.model, 'artist'])

same

@@ +255,3 @@
                     else item.get_favourite(), category, composer])
+            self.cache.lookup(item, ArtSize.small,
+                              self._on_lookup_ready, _iter)

just _iter on the next line

@@ +266,3 @@
                                   _iter)
+                self._artists[artist.casefold()] = {
+                                              'iter': _iter, 'albums': []}

indent on the comma, for as much fits on the line within 79 chars

@@ +299,3 @@
         cols[0].reorder(cells[0], -1)
+        cols[0].set_cell_data_func(
+            cells[0], self._on_list_widget_selection_render, None)

ditto

@@ +471,2 @@
+        self.head_iters = [albums_iter,
+                           artists_iter, songs_iter, playlists_iter]

indentation all above

@@ +472,3 @@
+                           artists_iter, songs_iter, playlists_iter]
+        self.songs_model = self.model.filter_new(
+            self.model.get_path(songs_iter))

pre

@@ +476,2 @@
         # Use queries for Tracker
         if not grilo.search_source or \

see the other if statement, parenthesis around multi argument statements + operator on start of the line.

@@ +480,3 @@
                 query = query_matcher[category][fields_filter](search_term)
+                grilo.populate_custom_query(
+                    query, self._add_item, -1, [self.model, category])

indentation

@@ +483,2 @@
         if not grilo.search_source or \
            grilo.search_source.get_id() != 'grl-tracker-source':

if statement, see above

Thanks for your effort, I cleaned searchview.py . It does need a proper rewrite to really clear things up.