-
-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uploaded Artists: fix fetching and displaying uploaded artists #107
Uploaded Artists: fix fetching and displaying uploaded artists #107
Conversation
@mikooomich a fix from a missing piece from the big PR |
0ebd7a2
to
267b158
Compare
ef1a0fe
to
a95972e
Compare
a95972e
to
da1032c
Compare
@mikooomich i'll take a look. Also here I've added two sync fixes:
|
da1032c
to
fe215a3
Compare
@mikooomich I think I've found the problem and fixed it! |
@mikooomich also, after adding the corutines to sync songs faster. I found a kind of problem caused by that: the songs are added in a different order compared to what YTM returns. So, if you sort by date added, it's not identical to what YTM responds. This I think is fine except for likes, I find it annoying that if I order songs by liked date, it's not identical to YTM. What do you think? Should we revert the corutine at the end of syncLikedSongs() and just leave a for each for adding the likes? Another option I see is to do first the toggle like logic linearly and then insert/update with a corutine |
IIRC toggle like just assigns a timestamp, so that would be a best of both worlds solution. Though, I'd have some thread limit (ex. thread pools) for jobs like this, if dispatchers doesn't do this automatically. But also... Does coroutines can provides significant performance boost over just a simple for? The db transactions are processed sequentially anyways. I'd imagine you put the I haven't had the time to look at this too closely, but in general, do what you think would be best; you probably are more familiar with this code than me. Thanks for doing the sync work I dread lol |
- If lost connection, change to local screen - If no connection, don't change to remote - fix local header being ellipsed when it shouldn't
350375a
to
eed6164
Compare
@mikooomich okey. For now, I'll let the likes sync as a simple for loop. Later we can improve this. If the rest of the changes seem ok could you merge the PR? I have other changes on a branch based on this one but related to other stuff that i would like to open a PR. |
4060eb3
to
5333c96
Compare
if (!showLocal) { | ||
showLocal = true | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattcarter11 do you know why this is done? Doesn't this always set showLocal to true? Currently, when online, only local content will be shown, and to my knowledge, the online content is never shown.
From what I gather, you want to show offline content when offline and online content when online?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I would like to merge in the compose 1.7 changes soon (since they'll probably end up conflicting badly), are you ok with me merging that in after your 3 prs (#127 #125 #121)?
Here is the branch with compose changes rebased on current dev https://github.com/mikooomich/OuterTune/tree/canary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikooomich ow, it's missing some logic, here's a fix: #128
About the merge, yes, currently I don't have new code to PR (but I will) so all good for me
Before, you could not fetch and display the privately owned (uploaded) artists
Uploaded artist
YouTube artist
Also fixed: