Skip to content
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

Resize indicator icons to fit panel #43

Merged
merged 1 commit into from
May 14, 2019
Merged

Resize indicator icons to fit panel #43

merged 1 commit into from
May 14, 2019

Conversation

vkareh
Copy link
Member

@vkareh vkareh commented Apr 25, 2019

This fixes two issues:

  • Some indicators have icons that are not the correct size (e.g. scalable 256px) and so they clip on the panel and look ugly.
  • When resizing the panel, indicators stay the same size. This updates the indicator icons so that they resize along with the panel.

Fixes #29

@vkareh vkareh requested a review from a team April 25, 2019 18:44
@vkareh
Copy link
Member Author

vkareh commented Apr 25, 2019

Notice the VLC, SpiderOak and Slack icons.

Before:
before

After:
after

Resizing the panel should also auto-scale the icons now.

@raveit65
Copy link
Member

This should be reviewed by someone who use and build it for his distro.
Sorry, i never build or use it for fedora.

@vkareh
Copy link
Member Author

vkareh commented Apr 25, 2019

@raveit65 - I know, this doesn't apply to you ;)

Actually, I've never been able to successfully build this on Fedora, but I'm not that concerned about that

@vkareh vkareh requested a review from sunweaver April 26, 2019 00:50
Copy link
Member

@lukefromdc lukefromdc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I've only ever gotten this to actually show my indicators ONCE on Debian Unstable, the code looks mostly OK on first examination. I do see

/* Track panel resize */
	g_signal_connect(applet, "change-size", G_CALLBACK(entry_resized), io);

but don't see anything added disconnecting it when the applet is removed or the panel destroyed. Is that already taken care of for all signals used by the indicator applet elsewhere, as is sometimes the case? If so nothing to worry about. Test would be to add and remove the applet whiile watching syslog for any errors involving that signal.

I don't see any obvious memory management errors of the newly dynamically allocated variables (as opposed to new pointers to existing variables) not getting freed sort.

Just commenting as running this on Debian always gets "no indicators" even though I have network-manager-applet running in indicator mode (and it ought to support this). That is true even when the normal mate-panel tray applet(with its own SNI support) is removed from the panel.

@lukefromdc lukefromdc requested a review from a team April 26, 2019 03:03
@raveit65
Copy link
Member

Actually, I've never been able to successfully build this on Fedora, but I'm not that concerned about that

Fedora ships libindicator for building, but there isn't an indicator for using ;)

@selectiveduplicate
Copy link
Member

selectiveduplicate commented Apr 26, 2019

I've been waiting for this to be fixed for a long time! Thanks a lot...
I'll test this on a UM virtual machine as soon as I can, hopefully today... 🙂

@vkareh
Copy link
Member Author

vkareh commented Apr 26, 2019

@lukefromdc - since the signal is emitted by the applet itself, it will be disconnected automatically when the applet is destroyed. The main use-case for explicitly disconnecting the signal is when using GObject instances, so this should be memory-safe, as far as I understand it.

To get this running on Ubuntu 19.04 was a bit of a pain, but finally managed using the following flags:

./autogen.sh --prefix=/usr --disable-static --libexecdir=/usr/lib/mate-indicator-applet --with-ubuntu-indicators=yes --libdir=/usr/lib/x86_64-linux-gnu --sysconfdir=/etc --enable-shared=yes --disable-maintainer-mode

Copy link
Member

@selectiveduplicate selectiveduplicate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some icons still get cropped, for example in the attached image "qbittorrent" gets cropped while VLC is okay...

panel_ind_icons_

@vkareh
Copy link
Member Author

vkareh commented Apr 26, 2019

@selectiveduplicate - seems theme-specific. Every other theme I tried shows qbittorrent correctly, except for Ambiant. Maybe there's some padding issue in the Ambiant panel?

@muktupavels
Copy link

I would say that hard-coded padding #define PANEL_PADDING 8 is problem...

@vkareh
Copy link
Member Author

vkareh commented Apr 26, 2019

@muktupavels - true, I don't like it either, but it's the one thing that made it consistently look good across themes. Smaller padding and icons start clipping, more padding and icons look too small... I can increase the padding to take into account whatever other stray indicator icons do, but where to stop?

@muktupavels
Copy link

The main use-case for explicitly disconnecting the signal is when using GObject instances

Is that bad wording? Or what did you try to say?

You must disconnect signal if you know that object you connect to will live longer then user_data. You really don't want that callback is called with already destroyed/freed object/data.

If both things are gobjects you can use g_signal_connect_object.

Hard-coded values are rarely good, no? As you say - "where to stop?"... Answer is simple, don't hard-code. I am sure that you should be able to get available space / height.

@selectiveduplicate
Copy link
Member

@vkareh Yeah, seems to be theme-specific. All the other themes (the ones that mate-themes ships) work fine without any cropping...

This fixes two issues:
* Some indicators have icons that are not the correct size (e.g. scalable 256px)
  and so they clip on the panel and look ugly.
* When resizing the panel, indicators stay the same size. This updates
  the indicator icons so that they resize along with the panel.
@vkareh
Copy link
Member Author

vkareh commented Apr 29, 2019

@muktupavels - thanks, I was misunderstanding this. I can see now on Memory management of signal handlers what you're saying regarding gobjects. I've updated the connect call to using g_signal_connect_object.

Copy link
Member

@flexiondotorg flexiondotorg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a huge improvement! Confirming this as fixing the oversized icons with the added benefit that some indicators icons were also way too small and now they are correctly scaled.

@vkareh vkareh requested a review from lukefromdc May 7, 2019 17:06
@vkareh vkareh merged commit 5a1775f into master May 14, 2019
@raveit65 raveit65 deleted the resize-to-panel-size branch September 20, 2019 12:07
@jrbastien
Copy link

I know this change is closed but I think it introduced a problem with large panels. I used to have a nice Windows10 like panel but after upgrade to 19.10 all my indicators icons are as big as the application icons.

Windows10Style

I'm trying to build a version prior to this change to confirm this and to possibly adjust the code to have some proportional padding but I get this error: "configure: error: Either Ayatana Indicators or Ubuntu Indicators are required to build mate-indicator-applet."

Assuming I have either one of the two with default Mate 19.10, what am I missing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some tray icons are cropped vertically
7 participants