Border for "separator_menu_item" should be 0
ClosedPublic

Authored by vzapod on Jun 17 2019, 5:23 AM.

Details

Summary

This revision aims to fix a crash in Sequencer64 (and possibly other apps) with this message:

(seq64:25899): WARNING **: 09:40:24.716: Invalid borders specified for theme pixmap:
        /usr/share/themes/Breeze/gtk-2.0/../assets/line-h.png,
borders don't fit within the image

BUG: 404045

Test Plan

No visual changes nor crashes in other apps.

Diff Detail

Repository
R98 Breeze for Gtk
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
vzapod created this revision.Jun 17 2019, 5:23 AM
Restricted Application added a project: Plasma. · View Herald TranscriptJun 17 2019, 5:23 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
vzapod requested review of this revision.Jun 17 2019, 5:23 AM
zzag added a subscriber: zzag.Jun 17 2019, 8:27 AM

Can you add some meaningful commit message?

Can you add it yourself? Or propose a name for me. Besides that you should understand that I know nothing about themes and pixmap engine. Can you tell me if all 0s are good thing? Should not it me something connected with line-h.png size? Can you say that 1, 1, 1, 1 is no-no?

ngraham added a subscriber: ngraham.

Vlad is saying that this patch's description section needs a short explanation describing why the change is necessary. You can do this by clicking "Edit Revision, which is on the sidebar in the top-right, or under the Actions hamburger menu in the top-right.

Adding some more reviewers.

"Edit Revision, which is on the sidebar in the top-right, or under the Actions hamburger menu in the top-right.

I know how to do it. But really have you read https://wiki.gnome.org/Attic/GnomeArt/Tutorials/GtkEngines/PixmapEngine
down to:
"How to theme a GtkHSeparator with an image:"

It uses all 0s why?

I don't know, sorry.

vzapod retitled this revision from [RFC] Change border for "separator_menu_item" line-h.png to Border for "separator_menu_item" should be 0.Jun 17 2019, 1:28 PM
mthw accepted this revision.Jun 17 2019, 6:31 PM

Your change does fix the issue mentioned, so as long as this doesn't break anything, I would say it's good enough.
On the other hand, I only made one code change before, so I don't think, I am a right person for a review.
So even though I accept this, do wait for at least one more review from someone else. And please do add a "Summary".

This revision is now accepted and ready to land.Jun 17 2019, 6:31 PM
vzapod added a comment.EditedJun 17 2019, 6:53 PM
In D21869#481305, @mthw wrote:

Your change does fix the issue mentioned, so as long as this doesn't break anything, I would say it's good enough.

Do you know what border does? Or gtk 2 is to old for you? Also maybe it is better to take variant from another theme? What is second popular theme in the world?

Adding some before-and-after images to the Test Plan section would help since I'm still not totally sure what the issue actually is.

vzapod added a comment.EditedJun 17 2019, 7:04 PM

Adding some before-and-after images to the Test Plan section would help since I'm still not totally sure what the issue actually is.

There is no difference, I suppose. If the border cannot be rendered it became as in default file.

But I found other variants,

border = { 1, 0, 1, 0 }
border = { 0, 1, 0, 1 }
also work. Surprise.

mthw added a comment.Jun 17 2019, 7:09 PM
In D21869#481305, @mthw wrote:

Your change does fix the issue mentioned, so as long as this doesn't break anything, I would say it's good enough.

Do you know what border does? Or gtk 2 is to old for you? Also maybe it is better to take variant from another theme?

No, I don't know anything more than you do.

What is second popular theme in the world?

I would say the default theme in gtk: Adwaita

In D21869#481322, @mthw wrote:

I would say the default theme in gtk: Adwaita

Thank you! That theme does not have border at all! Hahhh)))
https://gitlab.gnome.org/GNOME/gnome-themes-extra/blob/master/themes/Adwaita/gtk-2.0/main.rc#L1760

It's still not clear to me what this actually fixes.

Can someone provide one of the following:

  • Before-and-after screenshots, if this patch results in visual changes
  • A clear description of the problem to be solved, if this patch does not result in visual changes
mthw added a comment.Jun 19 2019, 7:44 AM
  • A clear description of the problem to be solved, if this patch does not result in visual changes

Sequencer64 crashes with the following message:

(seq64:25899): WARNING **: 09:40:24.716: Invalid borders specified for theme pixmap:
        /usr/share/themes/Breeze/gtk-2.0/../assets/line-h.png,
borders don't fit within the image

I don't know how, but this patch fixes that.

Thanks, that's what I was looking for. Please edit the Description section to add some of that information.

For testing, I could not detect any regressions with a variety of GTK apps (gtk3-demo, Firefox, Inkscape, GIMP).

mthw edited the summary of this revision. (Show Details)Jun 19 2019, 8:09 AM
mthw edited the test plan for this revision. (Show Details)
ngraham accepted this revision.Jun 19 2019, 8:11 AM
This revision was automatically updated to reflect the committed changes.

Thanks again for the patch! May it be the first of many. :)

mthw added a comment.Jun 19 2019, 8:22 AM

Could this also land in Plasma/5.16? It's a crash fix after all.

In D21869#481760, @mthw wrote:

Could this also land in Plasma/5.16? It's a crash fix after all.

Yeah, it should also land in Debian. And as fast as possible... For example I use Debian 10 with Gnome... Somebody should write to https://bugs.debian.org/cgi-bin/pkgreport.cgi?pkg=breeze-gtk-theme;dist=unstable

And I really do not want to))

mthw added a comment.Jun 19 2019, 8:54 AM
In D21869#481760, @mthw wrote:

Could this also land in Plasma/5.16? It's a crash fix after all.

Yeah, it should also land in Debian. And as fast as possible... For example I use Debian 10 with Gnome... Somebody should write to https://bugs.debian.org/cgi-bin/pkgreport.cgi?pkg=breeze-gtk-theme;dist=unstable

And I really do not want to))

We can only affect what goes into Stable (5.16) and what goes into Trunk (master), we cannot tell Debian team what package versions they should use.

In D21869#481766, @mthw wrote:

We can only affect what goes into Stable (5.16) and what goes into Trunk (master), we cannot tell Debian team what package versions they should use.

But there is also debian patches https://sources.debian.org/patches/breeze-gtk/5.14.5-1/

I landed this in master only because I don't have 100% confidence in it because I don't fully understand the underlying issue. I tested it as working, but to me "tested, but don't fully understand the underlying issue" means master-only.

Feel free to bug your distros of choice to backport the patch.

zzag added a comment.EditedJun 19 2019, 9:58 AM

I don't like how this patch was reviewed. The underlying problem wasn't fully understood. If an application crashes, then the backtrace is required. I don't see it in the bug report.

(seq64:25899): WARNING **: 09:40:24.716: Invalid borders specified for theme pixmap:
        /usr/share/themes/Breeze/gtk-2.0/../assets/line-h.png,
borders don't fit within the image

Pixbuf engine prints that warning because sum of top and bottom border is greater than the height of line-h.png. The most interesting part is that pixbuf engine tries to adjust borders, i.e.

gtk-2-24/modules/engines/pixbuf/pixbuf-render.c#L638-654
  if (theme_pb->border_left + theme_pb->border_right > width ||
      theme_pb->border_top + theme_pb->border_bottom > height)
    {
      g_warning ("Invalid borders specified for theme pixmap:\n"
		 "        %s,\n"
		 "borders don't fit within the image", theme_pb->filename);
      if (theme_pb->border_left + theme_pb->border_right > width)
	{
	  theme_pb->border_left = width / 2;
	  theme_pb->border_right = (width + 1) / 2;
	}
      if (theme_pb->border_bottom + theme_pb->border_top > height)
	{
	  theme_pb->border_top = height / 2;
	  theme_pb->border_bottom = (height + 1) / 2;
	}
    }

@zzag Right now we have a critical lack of manpower for Breeze GTK, so any assistance in reviewing patches is greatly appreciated. :)

In D21869#481795, @zzag wrote:

Pixbuf engine prints that warning because sum of top and bottom border is greater than the height of line-h.png. The most interesting part is that pixbuf engine tries to adjust borders, i.e.

So there is a mistake in theme after all? Or not! Really could you be more explicit.