qml: show fallback image if media cover is not available

in PlaylistDelegate and ArtworkInfoWidget. Use MediaCover widgets in these controls so that edge cases where artwork is not loadable can be addressed more clearly.

added MRStatus::Reviewable label

changed milestone to %4.0

added Component::Interface: Qt label

approved this merge request

added MRStatus::Acceptable label and removed MRStatus::Reviewable label

added MRStatus::Accepted label and removed MRStatus::Acceptable label

MR Acceptance result

This MergeRequest has been Accepted! Congratulations.

MR acceptance checks details:

• MR should be considered mergeable by Gitlab
• Last pipeline should be successful
• MergeRequest should have at least one external review and/or vote
• All threads should be resolved, have votes and score > 0
• MergeRequest should have no activity (threads/votes) for (72h/72h)

This message was automatically generated by homer-bot.

added 263 commits

• 36e7244d...49fadff8 - 261 commits from branch videolan:master
• 75f95790 - qml: show fallback image if media cover is not available in PlaylistDelegate
• 380feb81 - qml: show fallback image if media cover is not available in ArtworkInfoWidget

Compare with previous version

enabled an automatic merge when the pipeline for 380feb81 succeeds

merged

No, we don't use RoundImage when rounding is not wanted. This needs to be reverted.

There is no such rule.

This can not work, image width is not control's width.

The image needs to be scaled according to the height control has, and the control needs to respect image's width since this is an expanding control.

MediaCover does not even have implicit size defined...

The image needs to be scaled according to the height control has, and the control needs to respect image's width since this is an expanding control.

Now the the cover will scale with control height, Do you have any real world case where this really matters?

Now the the cover will scale with control height, Do you have any real world case where this really matters?

Any cover art where width is greater than height.

mentioned in issue #28749 (closed)

mentioned in issue #28750 (closed)

mentioned in issue #28751 (closed)

mentioned in merge request !5921 (merged)

mentioned in issue #28753 (closed)

MediaCover does not have status property, Image also does not make sense with this change. Playlist delegate no longer displays shadow because of this.

It is pretty obvious that this merge request has never been tested properly. It single-handedly caused at least 4 regressions, it deserves to be nominated as the worst merge request since !3385 (merged).

, it deserves to be nominated as the worst merge request since !3385 (merged).

Such comment are unacceptable

it deserves to be nominated as the worst merge request

This kind of exaggerated and unhelpful comment has happened more than once, and it's getting old. Claiming the merge request "has never been tested properly" is baseless and dismisses the work that went into it. Regressions are a normal part of development and should be addressed with specific, constructive feedback—not dramatic statements.

Labeling this as the "worst merge request" is neither accurate nor professional. If you have real concerns, address them directly. This kind of repeated negativity is not productive and needs to stop.

Claiming the merge request "has never been tested properly" is baseless and dismisses the work that went into it

If you actually tested this merge request but did not notice any of these five regressions, then this is gross incompetence, which is worse.

Regressions are a normal part of development and should be addressed with specific, constructive feedback—not dramatic statements

1 regression is okay, 2 is tolerable. There is no way 5 regressions is "normal part of development".

Labeling this as the "worst merge request" is neither accurate

If there is another Qt merge request that caused five regressions since !3385 (merged) I will take back my word and apologize for inaccuracy.

If you actually tested this merge request but did not notice any of these five regressions

Almost all of them depends on some small intricacies of implementation which I was not aware of since I don't work on this part of UI and is mostly worked by you. You could've helped me and everyone can enjoy contributing to the project but no you would prefer to demean others only so that you can look good.

If there is another Qt merge request that caused five regressions since !3385 (merged) I will take back my word and apologize for inaccuracy.

Your Qt6 migration, many parts of the UI still remain broken. You're making it a d*ck measuring contest, which I will not be part of and I will not comment on this topic anymore.

You could've helped me and everyone can enjoy contributing to the project

That's what I did in !5921 (merged) (fixing the issues you created), which was no-one but your responsibility. But you sabotaged my request by opening a thread without justification and essentially created revert of a revert in !5940.

Your Qt6 migration, many parts of the UI still remain broken

If you happen to spot an issue, you are supposed to create an issue instead of complaining in a completely irrelevant merge request. Not to mention, it is utter nonsense to compare these two merge requests.

I will not comment on your insults, that will be reported to the administration.

That's what I did in !5921 (merged) (fixing the issues you created)

That's not fixing but, you just revert the changes without looking into actual issues and discussion. !5921 (merged) completely ignores the aim of this MR. !5940 has actual fixes.

What's the issue with the thread I opened? How trying to start a discussion a sabotage?

That's not fixing but, you just revert the changes without looking into actual issues and discussion

Reverting your changes fixes all the issues I opened, so that's actual fixing. Additionally, I also covered the fallback case when the image fails to load.

On the other hand, !5940 tries to fix but fails because of obvious reasons I listed there and also here. These are what I remember:

• Control needs to respect artwork width, RoundImage does not support fillMode.
• Twitching when switching media.
• RoundImage does not take care of window DPR properly when QQuickRenderControl is in use.

I also covered the fallback case when the image fails to load.

That's doesn't handle all the edge cases that MediaCover does. Not to mention the unification of all this handling under single control.

!5921 (merged) doesn't matter since you need RoundImage there any way for radius and all other points are already being discussed in !5940, but I won't comment this in !5921 (merged) anymore since You will again take it as an attempt to sabotage.

This could've more gracefully handled if you in good faith, wanted to discuss and contribute rather than attempt to demean others' contributions.

@chub:

Such comment are unacceptable

Leaving aside the anger- or frustration-fueled hyperbole, @fuzun's comments at least have some basis in technical facts. He is allowed to have an opinion on the quality of merge requests. Merge requests are not people, and are not protected by the CoC. Simply put, the comments may be uncomfortable and dislikeable, but not unacceptable.

Accordingly, you (@chub) had every rights to complain about the tone, but a peremptory sentence of this sort is only making things worse, as we now see in hindsight.

N.B.: Like most maintainers, I have not read the thousands of back and forth Qt-related MR threads that took place in recent months, so I do not know the background. That does not excuse @jagannatharjun 's subsequent behaviour but it is not helping.

@jagannatharjun

Regressions are a normal part of development and should be addressed with specific, constructive feedback—not dramatic statements.

People are expected to test their MRs before they submit. See also https://wiki.videolan.org/Sending_Patches_VLC/

While constructive feedback is appreciated, reviewers are under no obligation to make their feedback constructive (unless they double as mentors). To reject a merge, all you need is to prove that it is incorrect. There never was, is no, and will never be a rule that you must also provide a path forward. We have been through this argument already.

The reviewer might not know the path forward either, or they might not have the time and patience to explain them, or might have already made the explanations previously.

With that said, the rest is essentially slandering @fuzun. Some people, including myself, have been suspended for that sort of attitude.

Merge requests are not people, and are not protected by the CoC.

Communication is, that's the first line in CoC

People are expected to test their MRs before they submit.

The MR was tested as per by limited knowledge based on the scope of the MR. This module is largely handled by @fuzun. I was on vacation for 2 weeks following the merger of this MR otherwise I would have most likely caught this myself, as I've done countless time before.

While constructive feedback is appreciated, reviewers are under no obligation to make their feedback constructive

I'm not asking for it either way. The original comment by @fuzun violates point (2) of the CoC

the rest is essentially slandering @fuzun.

Those are just replies to his inappropriate behaviour.

mentioned in issue #28754 (closed)

Playlist delegate is now completely broken with Qt 6.2 (#28754 (closed)), another regression, making it 5 regressions...