Rachel030219 / Poweramp-LRC-Plugin

An app to implement local .lrc file support for Poweramp.
Other
80 stars 4 forks source link

Searches for lrc Lyrics when embedded is enabled but no embedded lyrics is found #28

Closed fei0316 closed 3 years ago

fei0316 commented 3 years ago

Should resolve problem 2 of #26 Please do clean it up though, as the current implementation might not be very elegant (converts Uri to String, change the extension, then converts it back to Uri).

fei0316 commented 3 years ago

Oh it is for the lyrics color picker. The library you used only has English strings, and apparantly you could add translation to a library's strings by adding a string in the app's strings.xml file. Not sure if it is the best way though, but I think the library has not been updated for a long time already.

Rachel030219 commented 3 years ago

That's great ;) I'll build and test these changes later tonight (UTC+08:00).

Rachel030219 commented 3 years ago

Is it normal that commits from 9 days ago are also included in this pull request?

fei0316 commented 3 years ago

I think it is related to last time when you merged my PR on preference menu: previously you just directly let my commits in, and in those cases there are no problems; last time you probably did something differently that caused the message fei0316 authored and Rachel030219 committed 8 days ago, instead of the usual fei0316 committed 19 days ago. I just did a quick search and found this https://stackoverflow.com/questions/25327743/what-flow-causes-github-commits-that-are-authored-by-one-user-but-committed. Sorry I'm not that familiar with Git, but I guess I could remove those and update my fork to match yours instead

fei0316 commented 3 years ago

I reset my fork, stashed my changes, pulled from upstream, unstashed the changes, then committed and force pushed. Should look much cleaner now

Rachel030219 commented 3 years ago

Alright ;) To say briefly, there are three ways to merge a pull request. Squashing combines all commits into one commit. Creating a merge commit, as what I've done previously, moves full commit history into the branch and adds a commit on top of that (which makes the commit history messy). Rebasing (authored and committed by different users) makes it easy to recognise what've done while keeps the commit history as much as needed, hence my switching from merge commit to rebasing. As to this PR, I could only squash or create a merge commit, while rebasing introduces conflicts, and that's weird. The commits from a previous PR might be the reason, but since I'm not very familiar with git either, I have no idea but to discuss here.

Rachel030219 commented 3 years ago

Thanks for your effort!

fei0316 commented 3 years ago

Thanks for your explanation! I believe it is my fault actually. Squashing and rebasing would create commit(s) by you right after the original HEAD, which is where my fork's branch is based upon also. But since I already created commits after the original HEAD (the content of my PR), my fork would be incompatible with your master branch after merging. Then if I were to add more changes to my same branch, it would cause a conflict. The "correct" way to avoid this, I believe, is to create a new branch for every PR instead of reusing the same (which was what I did).

fei0316 commented 3 years ago

I think my implementation does not test whether an lrc file exists... Just now it crashed when the file does not have embedded lyrics not an lrc file.

Rachel030219 commented 3 years ago

That is a problem, and I'm still looking for a better solution.