Update README.md - Githubissues

tuplasuhveli commented 5 months ago

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.

Olf0 commented 5 months ago

@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.

tuplasuhveli commented 5 months ago

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.

Olf0 commented 5 months ago

The changes you made look good to me in general, @Olf0!

Thank you.

The markdown formatting is how most people do it to enhance readability of the markdown source.
Headings look better if you only use every second size, when one needs only a few heading sizes, because the size difference between two consecutive sizes is rather small.

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.

Do you plan to get a new SailfishOS device so you may be able to review this in a while?

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.

Done by commit f55075f.

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.

Ack, also done by commit f55075f.

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).

"Beautiful" is extremely subjective and IMO "shameless bragging" :wink: (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).

tuplasuhveli commented 4 months ago

Do you plan to get a new SailfishOS device so you may be able to review this in a while?

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!

Olf0 commented 4 months ago

@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:

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).

_{(From the last paragraph of #106 (issuecomment-2197792672) three weeks ago.)}

tuplasuhveli commented 2 weeks ago

@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?

Olf0 commented 1 week ago

LGTM!

Well, sure you do, because you liked your changes and my changes on top of it. :wink:

Is there still something I need to do before this PR can be merged?

Yes:

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).

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.:

_{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.}

_{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).}

tuplasuhveli commented 1 week ago

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).

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".

I also asked myself what to do with this PR

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 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 envy them!}

Olf0 commented 1 week ago

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).

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?

Yes, they are only jump labels (HTML id tags, which can be appended to a URL of a web-page separated by a hash mark: #) on the same web-page. They simply jump to the position where GitHub's web-frontend expects specific comments as part of their review functionality. Yes, it is a bit confusing first, but this functionality allows to discuss specific code lines; once understood how to use it, it is quite handy.

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").

I also asked myself what to do with this PR

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.

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.

tuplasuhveli commented 1 week ago

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)

https://github.com/user-attachments/assets/68ab4c41-a226-4661-89d5-f21f2c71d049

Am 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:

Broken vs. deprecated: I think they share pretty much the same meaning, unless I'm mistaken; something that used to work but doesn't anymore. Broken might be a better term for being a more common word, but I don't have an issue with either one.
Lyrics support: Yes it does work, but it's not very intuitive: First of all, you have to have a song playing. Then tap on the small album cover on the low left corner. After that you need to swipe left to see the lyrics.

flowplayer

EDIT: Ah, the video isn't working. Let me come back to this later today.

Olf0 commented 1 week ago

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):

"broken" means "does not work; usually (but not always) it implies that it has been working before"
In contrast to that, "deprecated" means "it is currently being phased out, ultimately to be removed in the future; usually (but not always) this implies that it is old, and often (but not necessarily) also that it has been superseded by something newer".

HTH, and IMO these are clearly disjunct meanings: "deprecated" is something still working, "broken" definitely not.

P.S.: The video is displayed as: video-not-working But don't mind; except you want to resolve the review comment issue (I would be interested, but not at all strongly so).

Olf0 commented 6 days ago

Lyrics support [… is] not very intuitive

I created issue #111 for that.

I even tried to find my way to those review threads by browsing "Files changed" and "Commits" tabs, but I was unsuccessful.

Yes, they are also shown at the corresponding locations on the Files changed "tab" (in addition to the Conversation "tab"), but not on the Commits "tab" of a PR at GitHub for which someone triggered the Start a review functionality of GitHub's web-frontend.

Maybe some JavaScript filter (I use NoScript)? JavaScript must be allowed for github.com (plus fetch and ping) and githubassets.com (plus fetch).

P.S.: Now that I marked my review as finished, these are displayed in collapsed form below the last regular comment ("LGTM now.").

Olf0 commented 6 days ago

Now I also updated FlowPlayer's OpenRepos page with this content.

sailfishos-applications / flowplayer

Update README.md #106