fix(network-manager): not accepting char to display icons

[MrFantOlas]

Fixes #1423

As it is my first PR here it is hopefully not too bad.
Also note that I had no previous Gtk experience so if I did something atrocious, please point it out.

Questions that still remains:
Should all icons contain "icon:" ? From what I understand specifying a path should also be accepted. Maybe a better condition there would be good or a rule to differentiate icons from custom text.

Opening this as draft since I could not find a satisfying answer to that question.

[JakeStanger]

Should all icons contain "icon:" ?

Not if it's coming from the config. The system is designed to allow arbitrary text if preferred, or parse as different formats (http(s)://, file://, ...).

[MrFantOlas]

I would think that adding a custom key, like "raw:", would be easier to parse to display a label instead of an icon. This way we do not have to wait and see if the string resolves before showing as a label. This also avoids showing an unresolved path in the label.

I do not know however since this module is the only module that currently allows svg icons (from what I have read of the code) that this would be later expanded to other modules (and then maybe a better handling to avoid destroying peoples configs).

[JakeStanger]

I can see potential utility in adding a new raw: prefix to skip image parsing, but I'm not sure I'm following the rest, nor do I see how that ties into this particularly.

This also avoids showing an unresolved path in the label.

What do you mean by this?

this module is the only module that currently allows svg icons

All modules use the central image parsing system, which supports SVGs.

[MrFantOlas]

What do you mean by this?

I meant that when the path turns out unresolved you could assume it was not a path and show it in the label. But that seems like an horrible solution: you could get full paths in the bar, it does not give a clue to what happened, you have to wait before showing a label that the resolution is done.

All modules use the central image parsing system, which supports SVGs.

I am not sure about this. For example I was not able to parse an SVG image in the volume module. I was also unable to use the low and medium profiles there with the TOML config so maybe there is something that I missed there ?

Anyway I think I will try to make a clean "raw:" etiquette system as it is not far of from what I have already written.

[JakeStanger]

I meant that when the path turns out unresolved you could assume it was not a path and show it in the label.

This isn't the current behaviour though. If you pass a path with file://, it will fail to resolve the image, show blank and log an error, eg:

 WARN ironbar::image::provider: 143: failed to load image: zen: 
   0: Error opening file /does-not-exist.png: No such file or directory

For example I was not able to parse an SVG image in the volume module

Either a config issue or a bug. It's definitely supposed to be supported anywhere images are supported. There's specific support for it here:

ironbar/src/image/provider.rs

Lines 254 to 264 in 6e05d55

	Some(ImageLocation::Local(path)) if path.extension().unwrap_or_default() == "svg" => {
	let scaled_size = image_ref.size * scale;
	let pixbuf = Pixbuf::from_file_at_scale(path, scaled_size, scaled_size, true)?;
	let buffer = pixbuf.save_to_bufferv("png", &[])?;
	let bytes = Bytes::from_owned(buffer);
	let texture = Texture::from_bytes(&bytes)?;
	Ok(Some(texture.upcast::<Paintable>()))
	}

I was also unable to use the low and medium profiles there with the TOML config so maybe there is something that I missed there ?

What do you mean by "use"? Regardless they're definitely there, and they definitely work.

[MrFantOlas]

To be clear applying this config

[[end]]
type = "volume"
sink_slider_orientation = "horizontal"
source_slider_orientation = "horizontal"
format = " {icon} {percentage}% {name} "
max_volume = 100

[end.icons]
volume_high = "T"
volume_medium = "T"
volume = "icon:network-wired-symbolic"
volume_low = "T"
muted = ""

Only changes the high volume output to be a string of "icon:network-wired-symbolic". This is may be the wiki not being up to date. I have seen that in the code there are profiles implemented so there should be a way to make it definitely work. I think it would be best to have this discussion in another issue maybe (which I can open later).

The relevant part for this PR is that "icon:network-wired-symbolic" does not get translated to an SVG icon. I mention this because the method you linked is in used in the following modules: networkmanager, custom, launcher, menu, music and tray. (Searching for a reference of load_into_picture(_silent) as it is the only method that calls get_paintable)

All modules use the central image parsing system, which supports SVGs.

So I am unsure what you meant by all modules ?

I mention this because then it might be better to have a shared system (the :raw prefix) that applies to all of these modules and at the same time extend the support of SVG icons to other modules ?

[JakeStanger]

This is may be the wiki not being up to date

Ah yeah you're right. I'll log that to get fixed as soon as I can.

it might be better to have a shared system (the :raw prefix) that applies to all of these modules and at the same time extend the support of SVG icons to other modules ?

This is what already exists, which is what all image loading across all the modules already uses. It already supports SVGs, and that's what I linked above.

[MrFantOlas]

Ok I finally understood what you meant, sorry for having dragged this.

I think then that only the networkmanager module would benefit from this, therefore I can open this now :)

[JakeStanger]

Okay sorry this is the first time I've properly looked at the code, but I think this needs reworking.

There already exists an IconLabel struct which implements this logical, albeit inverted - it looks for an explicit prefix (icon:, file://, https://), and treats it as the raw string if none are present.

ironbar/src/image/gtk.rs

Lines 192 to 199 in 6e05d55

	pub fn set_label(&self, input: Option<&str>) {
	// Remove old icon if present
	if let Some(old) = self.current_icon.borrow_mut().take() {
	self.container.remove(&old);
	}
	match input {
	Some(input) if image::Provider::is_explicit_input(input) => {

Rather than implementing a new system, using that would be preferable

[MrFantOlas]

No worries. I have rewritten using the IconLabel.

I am not the most proficient with the gtk lib and the api so hopefully them being marked as immutable is true.

[JakeStanger]

Cool thanks, code lgtm. Can you fix the build issue please? Just a feature flag that needs adjusting.

Once that's done I'll give this a quick test and make sure it's all good, and go through wine a fine comb, then I think we're good to go.

I am not the most proficient with the gtk lib and the api so hopefully them being marked as immutable is true.

gtk-rs provides safe bindings so you can trust the compiler. If it doesn't ask for mutability, you're good. GObject structs are basically just pointers that get used for FFI, so they don't get modified in rust-land and can be cloned cheaply.

[MrFantOlas]

Just a feature flag that needs adjusting.

Yes I can see now. I have added it !

Edit: That was not the case, now it is. I had no idea you had to log into GitHub to see job details.

gtk-rs provides safe bindings so you can trust the compiler. If it doesn't ask for mutability, you're good. GObject structs are basically just pointers that get used for FFI, so they don't get modified in rust-land and can be cloned cheaply.

That's good to know 👀

[JakeStanger]

Briliant thanks, lgtm