PICARD-969: Queue search dialog xmlws to run first #620
Conversation
If you are loading a lot of albums and have a long queue of releases being looked up, then the xmlws request for Search for similar albums... is queued behind them. This commit runs these request as priority=True important=True which means that they will run as the very next xmlws.
|
Away for 2 weeks with email and web but no dev environment - other's can address review comments etc. or they can wait until I am back. |
picard/webservice.py
Outdated
| cacheloadcontrol=cacheloadcontrol, refresh=refresh) | ||
|
|
||
| def find_releases(self, handler, | ||
| xml=True, priority=False, important=False, mblogin=False, |
There was a problem hiding this comment.
The repitition of keyword arguments in the new code looks unnecessary, why not just pass **kwargs through?
There was a problem hiding this comment.
Because the search terms are also kwargs so this distinguishes between keywords which are Picard internal and those that are part of the search term.
@mineo I still believe that this helps readers distinguish between Picard internal kwargs and search kwargs, but I am not wedded to this.
Now back in the UK and if you still want this simplified after reading my justification, let me know and I will make the changes at some point next week.
|
Because the search terms are also kwargs so this distinguishes between keywords which are Picard internal and those that are part of the search term. But not wedded to this. If we still want it changed then it will have to wait for 3-4 weeks until I am back in the uk. Or someone else can change it for me. |
... so I can see what else needs reverting.
|
Ok - ready for re-review. |
There was a problem hiding this comment.
This seems to break the "Search for similar tracks" action:
Traceback (most recent call last):
File "./picard/ui/mainwindow.py", line 819, in show_more_tracks
dialog.load_similar_tracks(obj)
File "./picard/ui/searchdialog.py", line 377, in load_similar_tracks
**query)
File "./picard/webservice.py", line 497, in find_tracks
return self._find('recording', handler, kwargs)
File "./picard/webservice.py", line 477, in _find
value = escape_lucene_query(value).strip().lower()
File "./picard/webservice.py", line 66, in escape_lucene_query
return re.sub(r'([+\-&|!(){}\[\]\^"~*?:\\/])', r'\\\1', text)
File "/usr/lib/python2.7/re.py", line 155, in sub
return _compile(pattern, flags).sub(repl, string, count)
TypeError: expected string or buffer
picard/webservice.py
Outdated
| @@ -453,7 +453,9 @@ def lookup_discid(self, discid, handler, priority=True, important=True, refresh= | |||
| return self._get_by_id('discid', discid, handler, inc, queryargs={"cdstubs": "no"}, | |||
| priority=priority, important=important, refresh=refresh) | |||
|
|
|||
| def _find(self, entitytype, handler, kwargs): | |||
| def _find(self, entitytype, handler, kwargs, | |||
There was a problem hiding this comment.
You could move the arguments that are only passed to self.get to kwargs here as well. Also, are there plugins that make use of this function? AFAICT all users set priority and important to True, so the defaults should reflect that.
There was a problem hiding this comment.
I suspect that the issue causing the error may be that sometimes we have **kwargs and sometimes we have only kwargs. though I will have to test this.
I am not quite sure what you mean by (but I expect I will work it out when I come to look at the code):
You could move the arguments that are only passed to self.get to kwargs here as well. Also, are there plugins that make use of this function?
I think your point about defaults is well made - I will make this change as soon as I get time to test it.
There was a problem hiding this comment.
I hope I have addressed all review comments - please let me know if I have missed any.
|
Ok - I think this is ready to merge now. Apologies for poor quality last time. |
|
Travis failure is because |
It's set to an empty dictionary later, ignoring its previous value.
It's set to kwargs later, ignoring its previous value.
picard/webservice.py
Outdated
|
|
||
| def _browse(self, entitytype, handler, kwargs, inc=[], priority=False, important=False): | ||
| def _browse(self, entitytype, handler, inc=[], | ||
| xml=True, priority=False, important=False, mblogin=False, |
There was a problem hiding this comment.
One more thing: why is _browse now neither important nor has it priority? It's used for the other versions context menu item, which seems to be of similar importance as the search for similar tracks...
There was a problem hiding this comment.
@mineo Thanks for the PR to the branch fr this in my repo. I will look at it more carefully as soon as I have time.
As for this comment - I agree and will change this at the same time as I review your PR.
Move keyword arguments that always use the default values to calls to self.get
|
@mineo please re-review so that this can be merged for 1.4.1 |
|
@Sophist-UK Just in case you're wondering what the status of this is: it looks good now and I'm ok with merging it, but I won't do it right now because using any of the internal search functions of picard immediately crashes it :( (reported as PICARD-1051). |
|
It missed 1.4.1 so there is no rush. |
…g-xmlws-to-run-first PICARD-969: Queue search dialog xmlws to run first
|
Don't forget to cherry-pick this to 1.4.x. |
Backport of #620 to 1.4.x Author - Sophist-UK
If you are loading a lot of albums and have a long queue of releases being looked up, then the xmlws request for Search for similar albums... is queued behind them.
This commit runs these request as priority=True important=True which means that they will run as the very next xmlws.
https://tickets.metabrainz.org/browse/PICARD-969