View and search image properties

[angstyloop]

Is there a "too early" for making a draft PR?

This is a draft, far from being complete. So far, I have added a "metadata" GSetting and menu item that toggles the visibility of an empty TextView inside a Popover to the right of the ImageWindow.

[angstyloop]

Progress: I am able to read and write the metadata fields of a VipsImage (yay).

However, they aren't being saved to the image file on SaveAs for some reason. It might be related to the exif-related warnings generated on SaveAs, which I show at the end of this short demo. I also seem to be adding an orientation field (and exif tag) that wasn't there before. Maybe these things are related.

So there are some issues I need to figure out still, but this is a good checkpoint, since I seem to have gotten over most of my issues reading/writing VipsRefString metadata fields.

I know that is looks weird to have the full text value of the VipsRefString fields be editable. It would probably make more sense to parse out the unparenthesized part of the string, make that editable, and then make the parenthesized part of the string readonly. Then cat them for the final string to write to the VipsImage. The parenthesizes part of the string could also be hidden entirely from view, but I don't know if that is going against the spirit of vipsdisp, since there is useful information in there.

[jcupitt]

Hey, this looks nice!

• Do you need the "apply" button?
• I think making the whole string editable is probably the best idea. We need better EXIF metadata handling in the libvips core, it's not something to fix in vipsdisp.
• You need to copy the image (with vips_copy()) before setting metadata to make sure you have a private VipsImage instance.
• I promise I'll spend some time looking at this properly, I'm still catching up from the crunch in June.

[angstyloop]

Thanks : ) I tried harder than I care to admit lol

• Do you need the "apply" button?

No, it's not necessary. It was a good checkpoint, since it saved me from having to write a bit of code. But I don't think it's much more work to make the updates automatically apply to the VipsImage in memory. I'll work on that soon.

I think the user should still have to Save As in order to see the metadata changes in their actual image file. Also, I think the app should tell them if they are closing with unsaved metadata changes. Not hard to implement, just details. Let me know if you're thinking something different though.

• I think making the whole string editable is probably the best idea. We need better EXIF metadata handling in the libvips core, it's not something to fix in vipsdisp.

Perfect - I'll leave it as is then.

• _You need to copy the image (with vipscopy()) before setting metadata to make sure you have a private VipsImage instance.

Okay thank you. I think that information is in the libvips source code and API docs, but I just didn't look hard enough.

• I promise I'll spend some time looking at this properly, I'm still catching up from the crunch in June.

Thank you - please don't feel any pressure. I still have stuff to work on, and I haven't even started code cleanup in this branch. You're doing me a favor by letting me poke around and ask questions, and eventually reviewing the code. Also, the code will likely not be perfect on the first round.

[angstyloop]

Update: I'm confident the vips_copy hint got me closer, but my incorrect usage of the API must run deeper than I thought. No big deal though - I'll just backtrack and look at the source code for vipsheader.c again to get a better idea of how to write metadata using the C API. That code is really clear, does what it's supposed to do, and is easy to test, so it should be an easy exercise for me at this point to pull a minimal example from it. I'll spend time on that this week, and add it to my small but growing collection of libvips examples.

If for whatever reason I can't manage that on my own, I'm just going to tidy it up and leave it for office hours with J : ) I'll technically be a full time student for like 8 weeks starting at the end of this month, so it will finally be quiet here, at least for a while. I'll try to have the code as close to reviewable as I can by then!

[jcupitt]

How about making a separate widget (Metadata? Header?) to go in the side panel? imagewindow.c is getting a bit unwieldy. I could have a quick go at the boilerplate for this, if you like. It'd be something like infobar.c I guess.

[jcupitt]

Haha, this works great!

I saw a couple more things:

1. You have two lines for the header. How about shrinking this to one line? Get rid of "Apply", have the search box on the right, and keep "Metadata" (or maybe "Header"? Or "Properties"?) in the centre. You could get rid of "Close" as well, or perhaps swap it for an X.
2. It needs a draggable divider (like eog's). You'd need a GtkPaned for the main image area, with the right-hand pane being the metadata area.
3. The mousewheel keeps changing the spinbox values :( We'd need to disable this misguided gtk feature.
4. Various small resize and layout issues I'm sure you're aware of.

[jcupitt]

glade can be useful for prototyping UI files, though I always end up polishing them by hand.

Did you find ctrl-shift-i? In any gtk4 app, it pops up an inspector you can use to view and change properties, very handy for tweaking values.

[angstyloop]

I suddenly became really busy a week ago. It feels like time has dilated and a whole month passed, but it's only been a week - does that ever happen to you?

Again, thank you for finishing up SaveOptions and referencing my PR. It's an honor and a privilege to have my name attached to that feature in any capacity!

How about making a separate widget (Metadata? Header?) to go in the side panel? imagewindow.c is getting a bit unwieldy. I could have a quick go at the boilerplate for this, if you like. It'd be something like infobar.c I guess.

Okay! I don't think I need the boilerplate though - I can just look at infobar.c and do my best. I was worried I would waste your time by not doing the GTK things perfectly, but you're unstoppable man. I don't feel bad for not being perfect! Let me give it a shot.

Haha, this works great!

I saw a couple more things:

1. You have two lines for the header. How about shrinking this to one line? Get rid of "Apply", have the search box on the right, and keep "Metadata" (or maybe "Header"? Or "Properties"?) in the centre. You could get rid of "Close" as well, or perhaps swap it for an X.
2. It needs a draggable divider (like eog's). You'd need a GtkPaned for the main image area, with the right-hand pane being the metadata area.
3. The mousewheel keeps changing the spinbox values :( We'd need to disable this misguided gtk feature.
4. Various small resize and layout issues I'm sure you're aware of.

Thanks J - feels good. Okay, I think I can handle these tasks. I'll work VISPDISP back into my meager schedule.

glade can be useful for prototyping UI files, though I always end up polishing them by hand.

Did you find ctrl-shift-i? In any gtk4 app, it pops up an inspector you can use to view and change properties, very handy for tweaking values.

The internet really wants me to use glade, but I don't feel like I've done it by hand enough yet to really appreciate glade, if that makes sense. Basically, I like suffering.

I did not know about the GTK4 inspector. That is an incredibly useful tool for debugging and testing! Thanks for the pro tip : )

[angstyloop]

I have time to work on this tomorrow through Monday - looking forward to it!

I have all the breadcrumbs I need, and I'm feeling motivated.

[jcupitt]

I added a few more things, you might want to merge master into your branch.

[angstyloop]

Today I started working on re-organizing the code related to the metadata viewer/editor into its own widget called Metadata. Since I'm still getting the hang of GObject classes and custom widgets, I referenced existing widget examples in vipsdisp. This involved a lot of copying code, changing the names of things, and moving definitions from file to file. The code isn't expected to compile yet.

I chose to reference the SaveOptions widget because it's easy for me to discern code specific to that feature from code required for all custom widgets, since I helped a bit with that code. InfoBar is still the simplest custom widget I would recommend other people look at in vipsdisp, though.

[angstyloop]

• Today I removed code from metadata.c and metadata.ui that was specific to saveoptions.c and saveoptions.ui, respectively.

• I moved some variable definitions and references from metadata.c back into imagewindow.c, and vice-versa, as needed.

• I also moved the XML that defines the Metadata widget from imagewindow.ui into metadata.ui

• There are still a lot of references to the ImageWindow variable "win" in metadata.c. If those are to be kept, then metadata.c will need to share similarities with infobar.c, so that the Metadata class can have an ImageWindow "win" member variable.

• The image_window_metadata function in imagewindow.c still needs to be updated to use the new metadata_new function defined in metadata.c.

• The code is still not expected to compile at this point, since there is still a lot to do in this refactor.

[angstyloop]

Today I made metadata.c more like infobar.c, to give the Metadata class access to ImageWindow methods.

• I made Metadata parent class GtkWidget instead of GtkDialog.

• I installed a GtkWindow "win" property on Metadata class

• I handled TileSource and ImageWindow change in Metadata class.

Besides that, I removed a few lines of dead code, and updated a few variable names.

The code still is not ready to compile. I still need to do some things:

• I need to go back through metadata.c and make sure Metadata class methods are using the new "win" ref correctly.

• I need to update image_window_metadata function in imagewindow.c to use the metadata_new method.

[angstyloop]

• In metadata.c, I used an ImageWindow "win" pointer on the Metadata widget - passing a Metadata object pointer around as an argument to Metadata methods - instead of passing around an ImageWindow pointer directly.

• In imagewindow.c, I passed a Metadata pointer to Metadata methods instead of an ImageWindow pointer.

• I initialized the Metadata widget in the metadata_init function instead of the image_window_metadata handler, which is now only responsible for showing or hiding the metadata widget.

• I updated the metadata_grid of input widgets dynamically, using the metadata_tile_source_changed handler.

• I set the orientation of the main_box in imagewindow.ui, instead of in image_window_metadata.

• I added some comments to image_window_metadata, metadata_init, and metadata_tile_source_changed, explaining what the code does.

With these changes complete, the Metadata widget should be a widget now. Like InfoBar, it has access to ImageWindow data and methods, and it knows when a new VipsImage is loaded. This means it can keep itself updated, so ImageWindow is only responsible for showing/hiding the Metadata widget, not creating/destroying it.

I made these changes following InfoBar, which also depends on ImageWindow, and so has good examples of the GObject boilerplate code needed to make this logic work.

That said, this code is probably still not ready to compile. I'll tackle that next time, and make it so that the Metadata widget functions the same as it did before, except now it's actually a Metadata widget.

[angstyloop]

Also, I haven't forgotten about the other tasks!

[angstyloop]

Finally, Metadata is a real GtkWidget, and the feature at least has the same functionality as it did before the refactor. Like InfoBar, Metadata updates its ImageWindow pointer, as well as its grid of user input widgets, whenever the TileSource held by ImageWindow changes.

Next up are these items:

1. Make the header a single line.

   • Get rid of "Apply"
   • Put the search box on the right
   • Keep title in the center
   • Change title from "Metadata" to "Properties"
   • Replace "Close" with an "X"

2. Make a draggable divider (like Eye of Gnome).

   • Use, e.g., a GtkPaned for the main image area, with the right-hand pane being the metadata area.

3. Prevent the mousewheel from changing the spinbox values.

4. Fix various small resize and layout issues.

[jcupitt]

Oh nice! Well done for pressing on with this.

I made a PR for background update of infobar https://github.com/jcupitt/vipsdisp/pull/22 which makes zoom quite a bit smoother.

[angstyloop]

Thanks! It's been fun learning more about the GObject, GTK, VIPS. VIPSDISP is a pretty neat application, and I don't even really use it for what it's really good at.

I merged your upstream changes into my fork, and nothing broke.

I'll see what I can knock out tonight :)

[angstyloop]

I made the made the Metadata widget behave more like the Infobar. Instead of having a Close button, the Metadata view is controlled by a persistent setting in the dropdown menu. There doesn't need to by an Apply button either, since the user still needs to click SaveAs.

[angstyloop]

Changed "Metadata" to "Properties", in the dropdown menu and metadata widget title.

[jcupitt]

Used a clever event controller to undo spin button scroll events.

Ouch! That's painful. Many other widgets will also respond to scroll events (option menu, slider, etc), will they need patching too?

nip2 addresses this by hooking into event propagation and blocking all scroll events reaching the window. I don't know if that's possible in gtk4.

[angstyloop]

Oh hey, I figured it out. I must of had something wrong before. I'll experiment to try and figure out my subtle error earlier.

I was looking at gtkspinbutton.c on gitlab and confirmed that GtkSpinButton sets up its EventControllerScroll and listens for "scroll" the same way I was before. So I reverted my code back to that, just to see if it worked, and it did!

[angstyloop]

Disable scrolling on a GtkSpinButton in GTK4.

/* No-op to prevent @w from propagating "scroll" events it receives.
 */
void disable_scroll_cb( GtkWidget *w ) {}

/* Disable scroll on a widget by adding a capture phase event handler and
 * connecting a no-op callback to the "scroll" event.
 */
static GtkWidget *
disable_scroll( GtkWidget *w )
{
        GtkEventController *ec;

        ec = gtk_event_controller_scroll_new(
                        GTK_EVENT_CONTROLLER_SCROLL_VERTICAL );

        gtk_event_controller_set_propagation_phase( ec, GTK_PHASE_CAPTURE );
        g_signal_connect( ec, "scroll", G_CALLBACK( disable_scroll_cb ), w );
        gtk_widget_add_controller( w, ec );

        return w;
}

/* Create a spin button with range, default value, and optionally enabled
 * scrolling.
 */
GtkWidget *
create_spin_button( double min, double max, double step,
                double value, bool scroll )
{
        GtkWidget *sb;

        sb = gtk_spin_button_new_with_range( min, max, step );
        gtk_spin_button_set_value( GTK_SPIN_BUTTON( sb ), value );

        return scroll ? sb : disable_scroll( sb );
}

[angstyloop]

Disable scrolling on a GtkScale in GTK4.

/* Create a scale with range, default value, and optionally enabled
 * scrolling.
 */
GtkWidget *
create_scale( double min, double max, double step,
                double value, bool scroll )
{
        GtkWidget *s;

        s = gtk_scale_new_with_range( GTK_ORIENTATION_HORIZONTAL, min, max, step );
        gtk_range_set_value( GTK_RANGE( s ), value );

        return scroll ? s : disable_scroll( s );
}

[angstyloop]

Remaining metadata tasks:

• draggable diivider
• sizing and layout
• different autocorrect algorithm (Levenshtein)
• assess metadata writing (works for some properties, but not others)

[angstyloop]

The draggable divider behavior isn't set in stone, but it's worth exploring to get a feel for the target behavior. Same goes for the layout and sizing.

[jcupitt]

Oh nice! It's getting closer!

There are still some obvious resize issues (I'm sure you know):

[jcupitt]

Maybe right-justify the label text? They can get pretty far apart right now, making it hard to see which label matches which widget:

eog also uses paler text for labels, which is nice:

[jcupitt]

I had a look thorough the code and I think metadata needs a bit of revising.

A widget shouldn't care what it's inside -- they need to be self-contained and only look "inwards", if you see what I mean. If I'm using a widget, I want to be able to drop it in anywhere.

Therefore, the metadata widget should just concern itself with displaying and updating metadata, it shouldn't need to know that it's sitting inside a GtkPaned. All the reveal / hide machinery needs to sit in imagewindow.c, since it's the thing (in imagewindow.ui) that makes the paned widget. For reveal / hide, I think all you need to do is set the pane position, and maybe set visibility if you want to get fancy.

I could have a go at a PR if you like (or maybe you'd rather change it).

[angstyloop]

I had a look thorough the code and I think metadata needs a bit of revising.

A widget shouldn't care what it's inside -- they need to be self-contained and only look "inwards", if you see what I mean. If I'm using a widget, I want to be able to drop it in anywhere.

Therefore, the metadata widget should just concern itself with displaying and updating metadata, it shouldn't need to know that it's sitting inside a GtkPaned. All the reveal / hide machinery needs to sit in imagewindow.c, since it's the thing (in imagewindow.ui) that makes the paned widget. For reveal / hide, I think all you need to do is set the pane position, and maybe set visibility if you want to get fancy.

I could have a go at a PR if you like (or maybe you'd rather change it).

The Metadata widget doesn't use any GtkPaned methods, fortunately. Instead, it uses GtkRevealer methods supported by the GtkSearchBar created statically with XML in metadata.ui. The GtkPaned is created statically with XML in imagewindow.ui, and is not referenced by its start-child (the image area) or end-child (the metadata widget), which is good and aligned with your comments.

However, the Metadata widget does manipulate the size of the image window in its metadata_show and metadata_hide methods, which is perhaps not aligned with your comments. Besides, widgets that manipulate the size of their containing windows is kind of anti-GTK. I'm sure there is a cleaner approach that takes the sizing and layout responsibility away from the Metadata widget. Let me make some adjustments and then see what you think!

[angstyloop]

"eog also uses paler text for labels, which is nice"

I agree - that does look nice! There are some other CSS tweaks I'd like to add to the Metadata widget, too, so it will be nice to get the CSS provider machinery in place. I'll add that to my to-do list.

[angstyloop]

I basically finished the Levenshtein Distance algorithm implementation today. I need to modify it some more to be a drop-in replacement for the Baeza-Yates matching algorithm, but it will support the same highlight-as-you-search functionality that's in place now.

[jcupitt]

Ah nice.

Did you see About / Credits btw?

[angstyloop]

Oh, like the contributors list? Because that was pretty cool!

[angstyloop]

Reworked match to use Levenshtein distance for inexact matches. Inexact matches are only displayed if there are no exact matches. In that case, a label that reads "No exact matches" is also shown. Levenshtein provides better inexact matches than the previous algorithm.

Cleaned up code. Re-organized some code into separate files. Renamed some symbols.

[angstyloop]

The metadata widget no longer changes window size when shown or hidden, and no longer uses a GtkRevealer internally to animate show/hide.

Its "revealed" property is just tied to gtk_widget_show / gtk_widget_hide. The metadata widget now fills available space in its containing widget, e.g., a GtkPaned "end-child" container widget. The container is responsible for sizing and layout.

[angstyloop]

The metadata widget now has support for CSS, and the metadata input labels are colored gray.

[angstyloop]

Remaining stuff:

• sizing and layout (might be done, or close)
• investigate why some metadata fields revert when the image is re-opened after save, while others don't
• estimate work for XML editor

[jcupitt]

That looks really nice!

I noticed a few small things (I'm sure you have too):

• the labels lose their right justification when you start typing in the search box ... even if you delete the search text, they don't get right just back
• the "properties" header has a frame around it which could possibly go
• you could make it a bit more compact by putting the search box into the "properties" header, perhaps at the right
• the properties are properties of the image, not the window, so I think conceptually it'd be better to have the gtkpaned holding the imagedisplay area on the left and props on the right, with the display control bar and the info bar at the top level, something like:

Though editing the .ui file seems to have stopped metadata updating itself, so I guess metadata.c is making assumptions about where it is in the hierarchy.

[jcupitt]

You could even get rid of the properties header bar and just have a search bar. The placeholder text in the search widget could work as the title:

[jcupitt]

Sorry, last one, I'd be temped to get rid of the metadata error and warning areas and get metadata.c to display anything like that with the main imagewindow error system.

displaybar has an image-window property and uses that for this kind of thing, for example.

[jcupitt]

(my changes to the .ui files, in case it's useful https://github.com/jcupitt/vipsdisp/commit/6566307dc991242ed749e7f06f8e051967a89473 )

[angstyloop]

Thanks J. I’ll work on these later today 🤩

[angstyloop]

Made the proposed changes:

• Fixed label justification
• Removed Properties title
• Removed error notification code (Metadata has an image-window property that can be used to show errors if needed)
• Make InfoBar and DisplayBar the full width of the window, and below Metadata and ImageDisplay.

More comments:

• Since none of the actual properties on VipsImage can be persistently changed by vips_image_set, they should just be read-only.

• Since the metadata fields of type VIPS_REF_STRING are not persistently modifiable, that part of the UI should also be read-only.

• The metadata field "orientation" - an integer type - can be persistently changed. This and other fields deserve an actual input widget.

[jcupitt]

Since none of the actual properties on VipsImage can be persistently changed by vips_image_set, they should just be read-only.

You're right that things like width and height can't be changed (the image would go crazy!), but everything else can be modified persistently. I think the problem comes with destructive changes (like vips_image_set()) on shared objects.

libvips images are supposed to be immutable, so you can construct them, but once constructed, they are read-only. This restriction makes caching a lot simpler -- images can be shared and reused across threads without the application program knowing about it.

So ... if you vips_image_set() on an image which some other part of the system is also using without your knowledge, your changes can be reversed. The solution is to call vips_copy() on an image before making any changes -- copy has the special property of always making a fresh image reference and it guarantees that no one else has that pointer.

The sequence needs to be:

1. vips_image_new_from_file(), or vips_text(), or something that makes an image.
2. image = vips_copy(image); to get a guaranteed unique reference
3. vips_image_set() to change any metadata item in any way you like, except for the core properties like width and height that directly affect image geometry ... you can set interpretation, for example.
4. vips_image_write_to_file() and it'll write the image back with the modified metadata.

[jcupitt]

It looks nice!

I noticed another tiny issue -- panning with mouse drags in the image view area seems to be broken. Perhaps the thing to stop scroll wheel edits has done this?

[angstyloop]

Thanks J! I'm sure that's all in the documentation, but I think understand the recap.

I'll make a copy and try to use VipsImage methods as much as possible to update properties on the image. Then I'll redo my little tests and see what I can get to persist across app reload. I'll do some basic testing and make dangerous properties immutable in the UI. Dangerous properties include width, height, bands, and pretty much anything else that will cause target size errors to throw on save.

By the way, one thing I found is that I can set some properties like width and height persistently (meaning persistently over reload) when I use g_object_set. This causes issues when you try to save the image a second time. This should have the same concurrency-related issues as vips_image_set, so I'm not sure what's going on there.

I don't think it's the scroll wheel disabling code that's responsible for messing with the panning, since that code just adds an EventControllerScroll to every GtkSpinButton in the Metadata widget. Only those spin button widgets are affected. I think it's probably the changes I made to imagewindow.ui, which re-organized the GtkPaned layout to make the infobar and displaybar full width and centered on the bottom, that are causing issues now with panning.

[angstyloop]

Making the copy before applying changes was probably a good thing to do anyway, but it doesn't seem to have fixed the issues I'm having with saving changes to properties other than "orientation" to the image using the vipsset* family of methods.

I'm going to get pan working again and just make the UI immutable for now, except maybe orientation, since that's a better checkpoint. I'm not sure what the deal is and will have to look into it more later.

[angstyloop]

Wait, in jcupitt/vipsdisp latest I can't pan with mouse drags. Has something else broken it, maybe related to saveoptions?

[jcupitt]

Hmmm, I just rebuilt everything to double-check check and it's working here. Maybe you're running another version?