GNOME Bugzilla – Bug 671101
Improvements to ghex
Last modified: 2012-03-01 23:52:08 UTC
Bunch of various patches that I had when I had the goal of making ghex more fancy... Never got to finish that off...
Created attachment 208741 [details] [review] Use resources framework
Created attachment 208742 [details] [review] Add a .gitignore file
Created attachment 208743 [details] [review] Provide a better experience for the "Goto Byte..." dialog Set the entry to be focused by default, and make it so that activating the entry activates the default button.
Created attachment 208744 [details] [review] Use the generic marshaller The specific marshallers, along with glib-genmarshal, are deprecated.
Created attachment 208745 [details] [review] converter: Squash some compiler warnings These functions expect signed chars, not unsigned chars.
Hi Jasper, Thanks for the patches! Every bit of help is much appreciated. I've been hacking on ghex a bit over the past few months and doing releases. It's been unmaintained for way too long and could use a lot of love ...
Review of attachment 208741 [details] [review]: ::: src/Makefile.am @@ +15,3 @@ + +ghex-resources.h: ghex.gresource.xml + glib-compile-resources --target=$@ --generate-header --c-name ghex ghex.gresource.xml This breaks with srcdir != builddir builds. I think it needs at least --sourcedir=$(srcdir) passed to glib-compile-resources, possibly more. @@ -63,1 @@ ghex-marshal.list The new ghex.gresource.xml file should be disted, so that ghex-resources.c and ghex-resources.h can be built from tarballs. ::: src/ghex.gresource.xml @@ +2,3 @@ +<gresources> + <gresource prefix="/org/gnome/ghex"> + <file>ghex-ui.xml</file> There's an option to strip whitespace from xml files (preprocess="xml-stripblanks"), is it something we should be using here?
Review of attachment 208742 [details] [review]: I've been meaning to add git.mk to ghex, but haven't gotten around to it yet. In the mean time, having a manually generated .gitignore is a good idea, I think.
Review of attachment 208743 [details] [review]: Looks good.
Review of attachment 208744 [details] [review]: Looks good, just a minor nit. ::: src/hex-document.c @@ +359,3 @@ NULL, NULL, + NULL, Something wrong with whitespace here.
Review of attachment 208745 [details] [review]: Looks good.
Created attachment 208828 [details] [review] Use resources framework This better?
Review of attachment 208828 [details] [review]: Almost there! Did you try to build in a srcdir != builddir configuration? I just gave it a try and it's looking for ghex.gresource.xml in the build dir: glib-compile-resources --target=ghex-resources.c --generate-source --c-name ghex --sourcedir=../../src ghex.gresource.xml Failed to open file 'ghex.gresource.xml': No such file or directory With srcdir != builddir I mean a separate build directory, something like this: $ git clean -dfx $ mkdir build $ cd build $ ../autogen.sh $ make -j4 Also, with this patch there's a new warning about ghex_datadir: ../../src/ghex-window.c: At top level: ../../src/ghex-window.c:516:1: warning: ‘ghex_datadir’ defined but not used [-Wunused-function] ghex_datadir() was only used to figure out the directory to load the UI file from, so I guess this patch should also remove the now unneeded ghex_datadir definition. ::: src/Makefile.am @@ +12,3 @@ +ghex-resources.c: ghex.gresource.xml ghex-ui.xml + glib-compile-resources --target=$@ --generate-source --c-name ghex --sourcedir=$(srcdir) ghex.gresource.xml If you use $< instead of ghex.gresource.xml, make will prefix the file name with path to the file, which is exactly what we need to pass to glib-compile-resources. @@ +15,3 @@ + +ghex-resources.h: ghex.gresource.xml + glib-compile-resources --target=$@ --generate-header --c-name ghex --sourcedir=$(srcdir) ghex.gresource.xml Same as above.
Created attachment 208831 [details] [review] Use resources framework Aha, thanks for that. Just tested again, and this should work.
Review of attachment 208831 [details] [review]: Yep, out of source builds are working nicely now, thanks! One more thing I noticed -- the patch removes ghex-ui.xml from EXTRA_DIST, but it should stay there. The file is needed for generating ghex-resources.*, and if we don't dist it, tarball builds won't work. Good work, just fix ghex-ui.xml disting and push to master, please.
Attachment 208742 [details] pushed as cfa790f - Add a .gitignore file Attachment 208743 [details] pushed as 40ce7d7 - Provide a better experience for the "Goto Byte..." dialog Attachment 208744 [details] pushed as 02d36f5 - Use the generic marshaller Attachment 208745 [details] pushed as 27411c8 - converter: Squash some compiler warnings Attachment 208831 [details] pushed as 3999be8 - Use resources framework And done, thanks!