Bug 441672

Summary:	image forces Kirigami.Card to expand
Product:	[Frameworks and Libraries] frameworks-kirigami	Reporter:	Matej Starc <matej.starc>
Component:	general	Assignee:	Marco Martin <notmart>
Status:	REPORTED ---
Severity:	normal	CC:	nate, shzam
Priority:	NOR
Version First Reported In:	unspecified
Target Milestone:	Not decided
Platform:	Other
OS:	Linux
Latest Commit:		Version Fixed In:
Sentry Crash Report:
Attachments:	Files for the result in the image above.

Description Matej Starc 2021-08-28 19:46:25 UTC

Created attachment 141107 [details]
Files for the result in the image above.

SUMMARY
For some reason images that have bigger height that width glitch `Kirigami.Card` so it expands in `Kirigami.CardGridView` which results in columns overlapping.


STEPS TO REPRODUCE
Make a `Kirigami.CardGridView` with more than 4 `Kirigami.Card`'s in it that have image with bigger height that width. This is just an example that i made.

OBSERVED RESULT
Cards in same columns overlap eachother.

EXPECTED RESULT
I don't mind the idea of making a Card expand if the image is bigger but that means it has to actually work and not overlap. I think having some (easy) options for setting image size would be cool in `Kirigami.Card` (if it should expand, fill, ...)

SOFTWARE/OS VERSIONS
Linux/KDE Plasma: 
KDE Plasma Version: 5.22.4
KDE Frameworks Version: 5.85.0
Qt Version: 5.15.2

ADDITIONAL INFORMATION
https://i.imgur.com/j445YVF.png

Comment 1 sh_zam 2023-01-06 13:08:42 UTC

This seems to be due to lack of an attached `Layout.minimumHeight` in https://invent.kde.org/frameworks/kirigami/blob/master/src/controls/templates/AbstractCard.qml#L119-128. I can see a merge request started by you to address this, but I wonder why you didn't proceed with it? https://invent.kde.org/frameworks/kirigami/-/merge_requests/712

Comment 2 Matej Starc 2023-01-06 13:17:01 UTC

(In reply to sh_zam from comment #1)
> This seems to be due to lack of an attached `Layout.minimumHeight` in
> https://invent.kde.org/frameworks/kirigami/blob/master/src/controls/
> templates/AbstractCard.qml#L119-128. I can see a merge request started by
> you to address this, but I wonder why you didn't proceed with it?
> https://invent.kde.org/frameworks/kirigami/-/merge_requests/712

I won't get merged until kf6 or ever. Depends on whay the "og" kirigami devs decide (there was talk of removing CardsGridView completely)

Comment 3 sh_zam 2023-01-09 13:31:19 UTC

“Matej Starc” <bugzilla_noreply@kde.org> writes:

> <https://bugs.kde.org/show_bug.cgi?id=441672>
>
> — Comment #2 from Matej Starc <matej.starc@protonmail.com> —
> (In reply to sh_zam from comment #1)
>> This seems to be due to lack of an attached `Layout.minimumHeight` in
>> <https://invent.kde.org/frameworks/kirigami/blob/master/src/controls/>
>> templates/AbstractCard.qml#L119-128. I can see a merge request started by
>> you to address this, but I wonder why you didn’t proceed with it?
>> <https://invent.kde.org/frameworks/kirigami/-/merge_requests/712>
>
> I won’t get merged until kf6 or ever. Depends on whay the “og” kirigami devs
> decide (there was talk of removing CardsGridView completely)

I think this is more of a bug with Kirigami.Cards than CardsGridView. This bug
is reproducible even if you use the vanilla GridsView.

Comment 4 Matej Starc 2023-01-09 13:46:28 UTC

(In reply to sh_zam from comment #3)
> “Matej Starc” <bugzilla_noreply@kde.org> writes:
> 
> > <https://bugs.kde.org/show_bug.cgi?id=441672>
> >
> > — Comment #2 from Matej Starc <matej.starc@protonmail.com> —
> > (In reply to sh_zam from comment #1)
> >> This seems to be due to lack of an attached `Layout.minimumHeight` in
> >> <https://invent.kde.org/frameworks/kirigami/blob/master/src/controls/>
> >> templates/AbstractCard.qml#L119-128. I can see a merge request started by
> >> you to address this, but I wonder why you didn’t proceed with it?
> >> <https://invent.kde.org/frameworks/kirigami/-/merge_requests/712>
> >
> > I won’t get merged until kf6 or ever. Depends on whay the “og” kirigami devs
> > decide (there was talk of removing CardsGridView completely)
> 
> I think this is more of a bug with Kirigami.Cards than CardsGridView. This
> bug
> is reproducible even if you use the vanilla GridsView.

Its because cards expand out of their cell. This can be either be fixed by limiting the height of the card to the cell height, or allowing the user to customize the cell size (as I've done in my MR).

Comment 5 sh_zam 2023-01-09 13:50:08 UTC

“Matej Starc” <bugzilla_noreply@kde.org> writes:

> <https://bugs.kde.org/show_bug.cgi?id=441672>
>
> — Comment #4 from Matej Starc <matej.starc@protonmail.com> —
> (In reply to sh_zam from comment #3)
>> “Matej Starc” <bugzilla_noreply@kde.org> writes:
>>
>> > <https://bugs.kde.org/show_bug.cgi?id=441672>
>> >
>> > — Comment #2 from Matej Starc <matej.starc@protonmail.com> —
>> > (In reply to sh_zam from comment #1)
>> >> This seems to be due to lack of an attached `Layout.minimumHeight` in
>> >> <https://invent.kde.org/frameworks/kirigami/blob/master/src/controls/>
>> >> templates/AbstractCard.qml#L119-128. I can see a merge request started by
>> >> you to address this, but I wonder why you didn’t proceed with it?
>> >> <https://invent.kde.org/frameworks/kirigami/-/merge_requests/712>
>> >
>> > I won’t get merged until kf6 or ever. Depends on whay the “og” kirigami devs
>> > decide (there was talk of removing CardsGridView completely)
>>
>> I think this is more of a bug with Kirigami.Cards than CardsGridView. This
>> bug
>> is reproducible even if you use the vanilla GridsView.
>
> Its because cards expand out of their cell. This can be either be fixed by
> limiting the height of the card to the cell height, or allowing the user to
> customize the cell size (as I’ve done in my MR).

Yes, in my case I *had* limited the size of the Card to cellHeight, but then the
image (headerItem) expanded into the contentItem (see AbstractCard.qml in
Kirigami) because there is no Layout.minimumHeight for it or anything else for
that matter :)

Comment 6 Matej Starc 2023-01-09 14:08:29 UTC

(In reply to sh_zam from comment #5)
> “Matej Starc” <bugzilla_noreply@kde.org> writes:
> 
> > <https://bugs.kde.org/show_bug.cgi?id=441672>
> >
> > — Comment #4 from Matej Starc <matej.starc@protonmail.com> —
> > (In reply to sh_zam from comment #3)
> >> “Matej Starc” <bugzilla_noreply@kde.org> writes:
> >>
> >> > <https://bugs.kde.org/show_bug.cgi?id=441672>
> >> >
> >> > — Comment #2 from Matej Starc <matej.starc@protonmail.com> —
> >> > (In reply to sh_zam from comment #1)
> >> >> This seems to be due to lack of an attached `Layout.minimumHeight` in
> >> >> <https://invent.kde.org/frameworks/kirigami/blob/master/src/controls/>
> >> >> templates/AbstractCard.qml#L119-128. I can see a merge request started by
> >> >> you to address this, but I wonder why you didn’t proceed with it?
> >> >> <https://invent.kde.org/frameworks/kirigami/-/merge_requests/712>
> >> >
> >> > I won’t get merged until kf6 or ever. Depends on whay the “og” kirigami devs
> >> > decide (there was talk of removing CardsGridView completely)
> >>
> >> I think this is more of a bug with Kirigami.Cards than CardsGridView. This
> >> bug
> >> is reproducible even if you use the vanilla GridsView.
> >
> > Its because cards expand out of their cell. This can be either be fixed by
> > limiting the height of the card to the cell height, or allowing the user to
> > customize the cell size (as I’ve done in my MR).
> 
> Yes, in my case I *had* limited the size of the Card to cellHeight, but then
> the
> image (headerItem) expanded into the contentItem (see AbstractCard.qml in
> Kirigami) because there is no Layout.minimumHeight for it or anything else
> for
> that matter :)

The reason AbstractCard does not limit the height of the header is because there is no need to do it. If you want to limit the header size, you can just limit the size within your code. AbstractCard only offers the standard card layout (with extra functionality).