Save options menu

[angstyloop]

• New save options menu. Uses introspection on the VIPS save operation to choose the appropriate widgets and text to display for a given output image format.

• Removed some white space.

• Modified some line breaks.

This is a draft. Feedback is strongly desired. Thank you!

[angstyloop]

Testing with Valgrind

I tested with Valgrind memcheck and confirmed this code does not introduce any new Valgrind messages to VIPSDISP.

What is Valgrind?

Valgrind is a useful memory analysis tool for C code. It is very easy to install on Ubuntu 22.04 with the APT package manager. However, it can be intimidating to new developers, because it prints memory addresses, the names of functions from shared libraries, and arcane messages for the user that can be difficult to process even for the more experienced.

I think the most basic approach to testing your code for memory leaks with Valgrind is to run your app with Valgrind before you make any changes, and also run it afterward. Then, you compare the two log files. If they contain roughly the same timeline of warnings, then you can likely conclude that your code did not introduce any new leaks, or at least ones that Valgrind is capable of detecting.

When there are new messages, you have to try to interpret what Valgrind is telling you, or just make an educated guess. For these commits, I found the couple of leaks Valgrind complained about by looking through my usages of g_malloc for mistakes.

I think there are much smarter ways to use the detailed information Valgrind presents to you, but you can also basically just use it like diff. It's still really useful.

How I used Valgrind here

For example, on Ubuntu 22.04, I run valgrind memcheck in Terminal like this:

valgrind --leak-check=yes ./src/vipsdisp

Notes

Repetition of the same Valgrind testing, or additional Valgrind testing, would be highly appreciated.

Note that VIPSDISP doesn't have a Valgrind Suppression File (.supp) yet, and there are a couple of pre-existing messages from Valgrind on app startup and image load. Using the VIPS .supp file does not prevent these messages from appearing, but it was worth a try.

[jcupitt]

Great! Nice work @angstyloop!

I tried it very quickly and noticed a couple of things:

• As you type a filename, the dropdown flickers and flashes horribly, with a nasty lag as well. Perhaps updates need to be throttled, or previous updates cancelled?
• If I save to x.jpg I get an error class "jpgsave" not found. Are you constructing the saver name from the filename extension? You need to use libvips API for this, see vips_foreign_find_save().

[angstyloop]

Great! Nice work @angstyloop!

I tried it very quickly and noticed a couple of things:

• As you type a filename, the dropdown flickers and flashes horribly, with a nasty lag as well. Perhaps updates need to be throttled, or previous updates cancelled?
• If I save to x.jpg I get an error class "jpgsave" not found. Are you constructing the saver name from the filename extension? You need to use libvips API for this, see vips_foreign_find_save().

@jcupitt thank you!

• That text input widget and auto-suggest dropdown are part of the GtkFileChooserDialog widget. There were no code changes that should affect that behavior. I only grab the filename when the Save button is clicked. It behaves the same in both branches for me, but I don't think my experience is matching up with yours.

Would you mind making a short video to show me the difference in behavior in your local branches? Then I could also confirm the behavior matches what I see in my local branches. I'm sure there is a good explanation!

I'm not saying the dropdown can't be disabled somehow (maybe by setting the filter to an empty string?). I can always go that route. But it should at least look the same in both your branches, because it's just the out-of-the-box behavior.

• Yes, you are exactly right - I am just concatenating the filename suffix to the English word "save", which is wrong. Like you said, I should instead be using vips_foreign_find_save, which maps equivalent suffixes to the name of the appropriate VipsForeignSave.

[jcupitt]

Ah you're right, the master branch also behaves really badly with the filename completion dropdown. We should just leave it.

[jcupitt]

Sorry, I noticed something else, you need to put your changes into a branch in your repo.

Make a branch called save-options or whatever in your git, then make a PR from that branch into master on this repo. Master on your repo should always be a copy of master on the upstream.

[jcupitt]

(so you'll need to delete this PR and make a new one, but it should be simple)

[angstyloop]

Oh snap, you're right, I'll make a new PR today.

I'm also removing most of the white space changes - they were kind of bloating the PR.

[angstyloop]

Here is the redo:

https://github.com/jcupitt/vipsdisp/pull/15