-
Notifications
You must be signed in to change notification settings - Fork 7
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
Update README.md #106
Update README.md #106
Conversation
This is a base for a new README with all the essential features I could think of. I don't have a working phone right now, so the listing is not perfect. I have marked all the bits I was uncertain about with a question mark, so please feel free to correct, delete and add anything you see fit.
@tuplasuhveli, thank you very much: This closes a sore point in the README (also at OpenRepos). Despite knowing that you broke your phone, I have a few review questions (see above). If you do not know an answer, just denote that, then I will try to rephrase the corresponding statement with ambiguity to work around (literally) the knowledge-gap. |
The changes you made look good to me in general, @Olf0! Perhaps we can wait a bit until we are certain about Last.fm scrobbling or just leave it out for now and possibly fill in later. Looking at the code, we can erase the question mark after "Play queue" - although I'm not 100% sure it actually works, but I can't think of a reason why it wouldn't. One thing I would change is "Gapless playback (can be disabled)" to the original "Gapless playback (optional)", since gapless playback is disabled by default and "optional" just sounds more concise, to me at least. Funnily enough, your wording in the sub-title with "feature-rich" was my original idea, but then I hesitated, because it's a rather relative term (see cmus or quod libet, for example). Nevertheless, I find it suitable in the context of SailfishOS. |
… playback as "optional", as suggested in sailfishos-applications#106 (comment)
Thank you.
Do you plan to get a new SailfishOS device so you may be able to review this in a while?
Done by commit f55075f.
Ack, also done by commit f55075f.
"Beautiful" is extremely subjective and IMO "shameless bragging" 😉 (as in "the best looking app ever"); in contrast to that "feature-rich" is "relative" (to how many features other media players for SailfishOS have, especially Jolla's), hence it is better arguable. Please also answer these two review comments of mine, preferably directly in these two review threads: #106 (review-thread-1059273477) and #106 (review-thread-1059279023). |
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.
Looks logical to me!
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.
The wording looks better than originally, thank you!
Hey @Olf0, I already have a Sony 10 V waiting for support for SFOS. So yes, but it is up to Jolla, how long of "a while" it's going to be! |
@tuplasuhveli, thank you for your kind words and I am glad that you already have an Xperia 10 V in store, but please, please do click on the following two links and provide your input, so we can finalise this PR and ultimately merge it:
(From the last paragraph of #106 (issuecomment-2197792672) three weeks ago.) |
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.
LGTM!
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.
LGTM!
@Olf0 Sorry for taking so long to answer, I tend to get anxious about unfinished things that are overdue. That's why I have been avoiding this PR. Is there still something I need to do before this PR can be merged? |
Well, sure you do, because you liked your changes and my changes on top of it. 😉
Yes:
Please just open these two links in new tabs and provide for each a reply (short or long, answering my question or writing "I don't know", as you like). P.S.:
But then the overdue tasks become stalled forever!?! IOW, the unfinished things are never finished. More seriously: Please do not become anxious about open tasks, this is "normal". Yes, I had to learn to live with that (my own open tasks), too, but setting priorities will always let some tasks being relegated further down on one's todo-list. E.g. Jolla has well documented bugs in SailfishOS for more than 10 years, still they prioritise new features higher again and again, even detrimental "features" as destroying SailfishOS' beautiful UI in SailfishOS 3.0.0 (less swipes, more buttons, plus visual changes, all to make the SailfishOS-UI more Android-style). I think we both would become very anxious if we would be allowed to browse Jolla's internal bug-tracker, but those who can apparently have no issue with the huge backlog of bugs to fix. I also asked myself what to do with this PR (feeling still a bit anxious about open tasks), but then I decided to simply wait for a reaction and let it rest until then (also for getting used to not to care about things where I cannot or do not want to proceed without others). |
Okay, this is something I find confusing in GitHub's UI: I open those links on separate tabs, but I have no idea what I'm commenting on. Both links show me they same view as opening this PR - is it how it should be? I tried with different browsers, but the result is the same. If I hover my cursor over the link, it says "You left a review".
If you can merge it without my input, you can do it. If it is not possible without my input, I hope you can clarify my confusion I described above.
I envy them! |
Yes, they are only jump labels (HTML I made two screenshots and marked in each the point to be addressed with a red ellipse (it is always the last line displayed there, though may address additional ones above, but not in these cases), my open question in blue and where you should place your answer in green: For the other two review questions, I believe to have understood the intentions of your replies in the main discussion thread here, but feel free to also comment there: Anything which might be helpful for my understanding (even if it is a simple "Yes" or "Correct").
Technically I sure can merge at any time. But practice has shown that it is much better to get it right when discussing a PR (and amending it with additional commits, if deemed necessary) than trying to fix things later in another PR and review round. |
Alright, I'm already feeling a bit stupid here :D Thank you @Olf0 for putting your time and effort on this and thanks to your screenshots I at least know what I'm looking for. I recorded my screen while trying to open one of the review threads on my Firefox. I had similar results with Chromium. (I hope the quality is sufficient, I had to shrink the video a bit) github.mp4Am I missing something obvious here? I even tried to find my way to those review threads by browsing "Files changed" and "Commits" tabs, but I was unsuccessful. Answers to your questions, I will move these to the correct thdreads, when I'm able to find them:
EDIT: Ah, the video isn't working. Let me come back to this later today. |
That is fine, I have my answers. BTW (without consulting e.g. en.wiktionary.org, just how I perceive these terms being used in IT):
HTH, and IMO these are clearly disjunct meanings: "deprecated" is something still working, "broken" definitely not. P.S.: The video is displayed as: |
I created issue #111 for that.
Yes, they are also shown at the corresponding locations on the Maybe some JavaScript filter (I use NoScript)? JavaScript must be allowed for P.S.: Now that I marked my review as finished, these are displayed in collapsed form below the last regular comment ("LGTM now."). |
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.
LGTM now.
Now I also updated FlowPlayer's OpenRepos page with this content. |
This is a base for a new README with all the essential features I could think of. I don't have a working phone right now, so the listing is not perfect.
I have marked all the bits I was uncertain about with a question mark, so please feel free to correct, delete and add anything you see fit.