Hey, I'm a newcomer and I wish to work on this.

(In reply to Rithvik Patibandla from comment #1)
> Hey, I'm a newcomer and I wish to work on this.

Hey welcome, it just happened that yashdev approached me on IRC wanting to work on this before you came along.

I'd say 2 people working on the same issue is a bit of a waste of resources. Maybe you can drop by on IRC and I can help you pick another bug. Or just take any of the other open newcomer bugs.

Created attachment 340221 [details] [review]
window: Change sync call to async while assigning view

Review of attachment 340221 [details] [review]:

The async part is fine (with some minor improvements), however code-readability wise you can still do a lot here. So I added some comments to get you there.

Also, when you refactor code try to ask yourself if something is (still) really needed and functional. Clean it up if not.

::: gnomemusic/window.py
@@ +192,3 @@
+    
+    @log
+    def _count_songs_query_cb(self, connection, res, Query):

last argument is data

If you make this function internal (ill explain later) you can maybe name this simply query_cb or something, since it's part of a contained logical code space.

@@ +196,3 @@
+            cursor = connection.query_finish(res)
+        except Exception as e:
+            logger.error("Tracker query crashed: %s", e)

use the error logging like elsewhere for now, just c&p

@@ +198,3 @@
+            logger.error("Tracker query crashed: %s", e)
+            return
+        def callback(conn, res, Query):

whitespace before a new function, whitespace is your friend.. it guides the eye

Also the name 'callback' is too generic, please name it according to what it does. In this case maybe 'cursor_next_cb' or something.

@@ +199,3 @@
+            return
+        def callback(conn, res, Query):
+            if conn.next_finish(res):

next_finish should be in a try/except block. This part of the async call generates the error.

@@ +203,3 @@
+                if count > 0:
+                    self._switch_to_player_view()
+                    # To revert to the No Music View when no songs are found

This comment is in an odd place (should be aligned with the else or in the function block imo), afaics it doesn't add much either way. the code makes it clear that it is switching to an empty view here b/c the song count == 0

@@ +205,3 @@
+                    # To revert to the No Music View when no songs are found
+                else:
+                    if self.toolbar._selectionMode is False:

'if not', but since it is the only thing thats going on here you can do 'elif not <arg>'

As an added note (maybe a bit outside what you are doing here), I am wondering what this is good for. Afaik this only get runs on startup, then we should not be in selection mode and this check is pointless. Something worth investigating?

@@ +208,3 @@
+                        self._switch_to_empty_view()
+            else:
+                count = 0

What is this else clause good for? we set an internal var and then the function ends? So.. we do nothing with it.

And whitespace! this is the end of your internal function.. use whitespace to mark it as such

@@ +209,3 @@
+            else:
+                count = 0
+        cursor.next_async(None, callback, Query)

don't pass the query (see next comment block).

Also you split up the function logic in 2 pieces for no reason (the try/except block at the start and this): just keep it together. Easier on the eye when reading the code.

@@ +219,3 @@
+                cursor = tracker.query_async(Query.all_songs_count(), None, 
+                                             self._count_songs_query_cb, 
+                                             Query)

Try catch is unneeded, the error in an async query always is on the finish part

Why pass the query as data (last argument)? It is not needed, you don't use it anywhere (nor should you). Just make it None.

@@ +227,2 @@
+        else:
+            # Revert to No Music view if XDG dirs are not set

This comment is in the right place (see my earlier remark of the placing of a comment), however it is so obvious what is going on here that imo it is unneeded.

@@ +256,3 @@
+        self._count = 0
+        self._cursor = None
+        self._assign_views(self._count,self._cursor)

Just do the query here and make all callback functions internal to the _setup_view, then it makes logical sense: _setup_view is one contained function. 

count & cursor aren't global (class-wide) vars. Don't make them so.

Created attachment 340297 [details] [review]
window: Change sync call to async while assigning views

Created attachment 340301 [details] [review]
window: Change sync call to async while assigning views

Created attachment 340310 [details] [review]
window: Change sync call to async while assigning views

Review of attachment 340310 [details] [review]:

Some minor nitpicks and good to go.

::: gnomemusic/window.py
@@ +189,3 @@
         elif 'Previous' in response:
+            self.player.play_previous()   
+   

what changed here?

Whatever it is, it shouldn't be in this patch.

@@ +232,3 @@
+                    self._switch_to_empty_view()
+        
+        def songs_query_cb(connection, res, data):

the cursor_next_cb call you use conn, here you use connection .. choose one

@@ +240,3 @@
+             
+            cursor.next_async(None, cursor_next_cb, None)
+            return

return is unneeded here

@@ +246,3 @@
+            cursor = tracker.query_async(Query.all_songs_count(), None, 
+                                         songs_query_cb, 
+                                         None)

I'm pretty sure the last None should go after the songs_query_cb argument

@@ +250,2 @@
         else:
             # Revert to No Music view if XDG dirs are not set

just drop this comment

@@ +307,3 @@
     @log
     def _on_select_all(self, action, param):
+        if not self.toolbar._selectionMode:

This really is a separate patch (part of the overall cleanup), it has nothing todo with the async stuff. So just drop it for this patch.

Review of attachment 340310 [details] [review]:

::: gnomemusic/window.py
@@ +216,3 @@
         self.add(self._box)
         count = 0
         cursor = None

One more, there is no need for count & cursor to be in global scope here, they aren't used. Define them (if needed at all) in their respective functions.

@@ +246,3 @@
+            cursor = tracker.query_async(Query.all_songs_count(), None, 
+                                         songs_query_cb, 
+                                         None)

query_async has no return value

Created attachment 340344 [details] [review]
window: Change sync call to async while assigning views

Created attachment 340345 [details] [review]
window: Change sync call to async while assigning views

Review of attachment 340345 [details] [review]:

lgtm

Created attachment 340402 [details] [review]
window: Change sync calls to async while selecting/deselecting songs

Created attachment 340404 [details] [review]
window: Change sync calls to async while selecting/deselecting songs

Review of attachment 340404 [details] [review]:

the async is fine, don't forget about the rest while changing the whole function

::: gnomemusic/window.py
@@ +120,3 @@
+                count = conn.get_integer(0)
+                if not count > 0:
+                    if not self.toolbar._selectionMode and len(self.views) != 1:

let's not forget about basic pep-8 stuff

You could actually make this one big if statement.

I find the ordering a bit odd, iirc in the other case the first case is where count is > 0 (so results). Next we handle the case of no results.

Created attachment 340405 [details] [review]
window: Change sync calls to async while selecting/deselecting songs

Review of attachment 340405 [details] [review]:

don't be hasty

::: gnomemusic/window.py
@@ +63,3 @@
     def __repr__(self):
         return '<Window>'
+            

what changed here?

@@ +120,3 @@
+                count = conn.get_integer(0)
+                if count > 0:
+                    if (self.views[0] == self.views[-1]):

the if/else clause have both a sublevel if, just 1 state .. why not push it together?

@@ +148,3 @@
+            cursor.next_async(None, cursor_next_cb, None)
+
+        tracker.query_async(Query.all_songs_count(), None, songs_query_cb,                                       None)

this doesn't look right

Created attachment 340408 [details] [review]
window: Change sync calls to async while selecting/deselecting songs

Created attachment 340432 [details] [review]
window: Change sync calls to async while selecting/deselecting songs

Created attachment 340434 [details] [review]
window: Change sync calls to async while selecting/deselecting songs

trigger event-
launch gnome-music and press the tick mark, besides the search icon, and then press cancel.


good condition-

                if (count > 0):
                    if (self.views[0] == self.views[-1]):
                        ......

bad condition (that causes seg fault)-

                if (count > 0
                        and self.views[0] == self.views[-1]):
                    ......

Committed with some minor fixes, also moved the calls to Grilo. Thanks for your work.

Regarding the issue in comment #21 , it is reproducible. Looks like some object scoping issue icm with python, however since this is dealing with libgd items I'm gonna leave it be until we complete the move away from gd.

That also pretty much tackles all our tracker sync queries as far as I can see. There's one minor one left, which is already tackled in another patch set coming soon.