jcupitt / vipsdisp

Tiny libvips / gtk+4 image viewer
MIT License
130 stars 10 forks source link

Save options #15

Closed angstyloop closed 1 year ago

angstyloop commented 1 year ago

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.

Also removed trailing white space in imagesource.c and tilesource.c (while removing them from my own code with regex). I can redact those changes though if desired though.

angstyloop commented 1 year ago

Here is a small demo of the save options menu.

new_save_options_menu

angstyloop commented 1 year ago

Some notes on how Valgrind was used to test this code for memory leaks:

https://github.com/jcupitt/vipsdisp/pull/14#issuecomment-1541156625

jcupitt commented 1 year ago

Hi again, I finally had some time to compile and test this. Nice work!

I noticed a few small things in a quick tryout:

I'll have a poke about in the code.

jcupitt commented 1 year ago

I think I'd make saveoptions into true widget, like infobar. So saveoptions.h would have something like:

#ifndef __SAVE_OPTIONS_H
#define __SAVE_OPTIONS_H

#define SAVE_OPTIONS_TYPE (save_options_get_type())

G_DECLARE_FINAL_TYPE( SaveOptions, save_options,
        VIPSDISP, SAVE_OPTIONS, GtkWindow )

SaveOptions *save_options_new( GtkWidget *parent, VipsOperation *operation );

#endif /* __SAVE_OPTIONS_H */

And a few other member functions, I guess. You'd be able to remove a lot of the member functions you have now if you reuse the gtkwidget / gtkwindow framework.

Shall I have a quick hack in another branch? I could sketch out a framework you could then cherry pick into this PR.

jcupitt commented 1 year ago

I made a basic save options framework: https://github.com/jcupitt/vipsdisp/compare/master...save-options

It does all the stuff with dialogs popping up and down, and the actual save operation, but doesn't have any of your nice options code. Hopefully you can just drop your stuff into that.

jcupitt commented 1 year ago

... I merged your option widget create and readout code into #20.

What do you think? I'm inclined to close this PR and do the finishing up in #20. Your work (and the credit for it) and in there now.

I feel bad for not pointing you in a clearer direction months ago :( I should have realized you were not using GtkDialog.

angstyloop commented 1 year ago

I've been preoccupied for the past week or so - thank you for grabbing the baton and running with it! I am proud to have contributed in any small way - it feels like a big accomplishment.

The heart of my contribution was the part that created widgets, populated them with data, and read that data to build the SaveOptions menu. As for the rest, I felt no shame hacking together GTK solutions the best I could. I always kept in mind that you might have to gut it entirely, but honestly it worked out better than I could have hoped!

I learned a lot in the process, and I learned more still by reviewing your changes. Being able to work on my humble contribution independently was really good for me. Thanks for including that stuff and modifying it to make it actually usable in VIPSDISP. This a cool feature, and I'm definitely proud of it!

angstyloop commented 1 year ago

I will go ahead and close this PR, which has been subsumed by PR #20.