Preparations for migration to gtk3

[pitdicker]

It has been a dozen years since I last wrote more than a couple of lines in C, and this is my first time working with gtk...
But just for fun I would like to make dvdisaster look ok on high-dpi monitors, and migrating to gtk3 is a first step.

This follows migration the guide https://docs.gtk.org/gtk3/migrating-2to3.html

The first couple of commits switch from deprecated to new widgets. Then two commits to use accessor functions instead of direct access.

In 8774d3d I couldn't find a replacement to disable line wrapping for message dialogs, and thought this is not the most critical thing to keep.

Next is the harder part: switching drawing operations to cairo. The problem is not translating the old methods to their replacements. We have to switch to a different model where drawing only happens inside the expose callback.

The existing code expected to be able to draw to an area whenever there was something to update, and it had efficient update routines that draw on top of the already drawn curve/spiral. The new gtk3 model expects widgets to just completely redraw their contents when they need to update, and to do so during the callback. I removed the update routines and only kept the functions that redraw the entire curve/spiral. I doubt this code was performance-critical even back when it was written. And it doesn't play very nice with anti-aliasing.

One change I did make to not have the drawing operations be too wasteful is to split the drawing area of the curve and spiral in the read linear window. When the spiral has to be redrawn we don't need to redraw the curve and visa-versa.

All of this still compiles with gtk2 and every commit should compile on its own. All other changes have to happen all at once with the switch to gtk3. I have not figured out that part yet 🤣. For the next PR.

Setting as draft for now because I would like to test it some more. But when it comes to testing I have a problem: I don't really use dvdisaster for its intended purpose. I don't even have writable media and don't know what window the raw editor is. Could you help test that all tabs/windows look ok? For the graph area I did compare screenshots to make sure the axis where a pixel-perfect match.

[pitdicker]

I'll get the CI passing, hopefully later today. Probably need to set a higher gtk2 version that has the new widgets that replace the deprecated ones.

Edit: No, it was just a mistake 😄.

[pitdicker]

A lot of changes in two days... I think I've got the bugs ironed out of the switch to cairo, and it successfully compiles with gtk3 🎉.

Now I only have to figure out what dependencies are needed for the CI, and I'm afraid I have to get an Ubuntu setup for that.

[speed47]

Hey, thanks for picking up this longstanding todo item!

I've never really worked with gtk either (well, apart from patching things around in this project), and I think I started once or twice a "gtk3" branch, but as you said it's not just about replacing old funcs with new ones, some concepts are completely different between gtk2 and gtk3, and need to be rebuilt from the ground up. You seem to be further that I've ever been in my past attempts, so let's do it!

I don't even have writable media and don't know what window the raw editor is. Could you help test that all tabs/windows look ok? For the graph area I did compare screenshots to make sure the axis where a pixel-perfect match.

Sure, I think I might even have a few "test" CDs/DVDs on which I did put dvdisaster recovery data then physically damaged them to be able to test properly. I'll not have much time today but'll give it a go tomorrow.

About the CI, don't hesitate to push/force-push as much as you need to trigger the workflows. Currently the Linux one seems to be passing, but not the Windows or mac ones. For Windows it's using msys2/mingw-w64 to be able to produce a .exe, and figuring the dependencies is indeed a PITA. If you have something that compiles under Linux, which seems to be the case so far, don't bother too much with the Windows or macos parts, I'll handle those if needed.

[pitdicker]

Thank you! I can probably figure out the CI this evening.

[speed47]

I've booted an Ubuntu 22.04 live usb, compiled your branch and tried several things. Most of the panels & windows seem to behave correctly, which is excellent news! I only pinpointed two issues so far.

I'm having a weird behaviour on the "read" (and "scan") tabs, linked to the spiral and its labels. At each refresh, the spiral and the labels go up a few pixels, until they're not visible anymore. It seems to happen because in update_geometry, Closure->readLinearSpiral->my is decremented at each call, and this is used in the computation of GuiDrawSpiralLabel.
In the gtk2 code, it's set to some value before being decremented so in the end, it doesn't move between each call:

dvdisaster/src/read-linear-window.c

Line 293 in 3be61f4

Closure->readLinearSpiral->my = a->height / 2;

I'm also getting some SIGSEGVs with different backtraces each time, when enabling "read and analyse raw sectors" and running a scan on a disc with a lot of C2 errors. I suspect this is linked to the label text updates on the bottom of the window, which is updated a lot in this case (EDIT: probably not, it still crashes when commenting C2-specific labeling code). It looks like the memory is silently corrupted and at some point it ends up crashing. I haven't looked at it too much yet, but here are some backtraces for reference:

Thread 14 "generic thread" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffeffd2640 (LWP 30635)]
0x00007ffff68c02b6 in ?? () from /lib/x86_64-linux-gnu/libpixman-1.so.0
(gdb) bt
#0  0x00007ffff68c02b6 in  () at /lib/x86_64-linux-gnu/libpixman-1.so.0
#1  0x00007ffff68c7981 in pixman_region32_union () at /lib/x86_64-linux-gnu/libpixman-1.so.0
#2  0x00007ffff753c740 in cairo_region_union () at /lib/x86_64-linux-gnu/libcairo.so.2
#3  0x00007ffff76c8cac in  () at /lib/x86_64-linux-gnu/libgdk-3.so.0
#4  0x00007ffff7b17edb in gtk_widget_queue_draw_area () at /lib/x86_64-linux-gnu/libgtk-3.so.0
#5  0x00007ffff7b1e31a in gtk_widget_queue_draw () at /lib/x86_64-linux-gnu/libgtk-3.so.0
#6  0x00005555555ffc6b in GuiChangeSpiralCursor (spiral=0x555555d58ab0, segment=14) at src/spiral.c:223
#7  0x00005555555a1aa7 in show_progress (rc=0x7fffe400a000) at src/read-linear.c:535
#8  0x00005555555a4473 in ReadMediumLinear (data=0x1) at src/read-linear.c:1261
#9  0x00007ffff7390ab1 in  () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#10 0x00007ffff7090ac3 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:442
#11 0x00007ffff7122850 in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
(gdb) 

Thread 1 "dvdisaster" received signal SIGSEGV, Segmentation fault.
0x00007ffff68bcfb1 in ?? () from /lib/x86_64-linux-gnu/libpixman-1.so.0
(gdb) bt
#0  0x00007ffff68bcfb1 in  () at /lib/x86_64-linux-gnu/libpixman-1.so.0
#1  0x00007ffff68bfb8f in  () at /lib/x86_64-linux-gnu/libpixman-1.so.0
#2  0x00007ffff68c7981 in pixman_region32_union () at /lib/x86_64-linux-gnu/libpixman-1.so.0
#3  0x00007ffff753c740 in cairo_region_union () at /lib/x86_64-linux-gnu/libcairo.so.2
#4  0x00007ffff76c8cac in  () at /lib/x86_64-linux-gnu/libgdk-3.so.0
#5  0x00007ffff7b17edb in gtk_widget_queue_draw_area () at /lib/x86_64-linux-gnu/libgtk-3.so.0
#6  0x00007ffff7b1e348 in gtk_widget_queue_draw () at /lib/x86_64-linux-gnu/libgtk-3.so.0
#7  0x00007ffff7b1e580 in gtk_widget_queue_resize () at /lib/x86_64-linux-gnu/libgtk-3.so.0
#8  0x00007ffff79bbdac in gtk_label_set_markup () at /lib/x86_64-linux-gnu/libgtk-3.so.0
#9  0x000055555558454a in label_idle_func (data=0x7fffe4959b80) at src/misc-gui.c:45
#10 0x00007ffff7361c44 in g_main_context_dispatch () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#11 0x00007ffff73b72b8 in  () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#12 0x00007ffff73612b3 in g_main_loop_run () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#13 0x00007ffff79c8d2d in gtk_main () at /lib/x86_64-linux-gnu/libgtk-3.so.0
#14 0x000055555557cae7 in GuiCreateMainWindow (argc=0x7fffffffddec, argv=0x7fffffffdde0) at src/main-window.c:506
#15 0x000055555556d872 in main (argc=1, argv=0x7fffffffe108) at src/dvdisaster.c:1079
(gdb) 

If you want, you may also remove the -Wno-deprecated-declarations flag from configure, you'll see that a few gtk funcs are deprecated. But I would say this is optional, it's probably better to focus having a working gtk3 build, even using a few deprecated funcs.

Thanks a lot!

[pitdicker]

Thank you so much for testing!

It seems to happen because in update_geometry, Closure->readLinearSpiral->my is decremented at each call, and this is used in the computation of GuiDrawSpiralLabel.

Oops, the function responsible for updating the geometry of the curve was changing the spiral center. And the re-use of my. Fixed.

I'm also getting some SIGSEGVs with different backtraces each time, when enabling "read and analyse raw sectors" and running a scan on a disc with a lot of C2 errors.

I can not reproduce the segfaults reliably, bud did manage it at least once and also got a few suspicious error messages. I suspect it was caused by a call to update the spiral widget from a different thread than the main thread, and is hopefully fixed now.

If you want, you may also remove the -Wno-deprecated-declarations flag from configure, you'll see that a few gtk funcs are deprecated.

Wow! Ca. 4000 lines of warning output. With the later commits I've got it down to 1200 lines by now, still some more to go.

[speed47]

Oops, the function responsible for updating the geometry of the curve was changing the spiral center. And the re-use of my. Fixed.

Can confirm this is fixed!

I'm also getting some SIGSEGVs with different backtraces each time, when enabling "read and analyse raw sectors" and running a scan on a disc with a lot of C2 errors.

I can not reproduce the segfaults reliably, bud did manage it at least once and also got a few suspicious error messages. I suspect it was caused by a call to update the spiral widget from a different thread than the main thread, and is hopefully fixed now.

I can no longer trigger the SIGSEGVs with the method I previously used. It would usually crash in less than 5 seconds after the scanning started, but now I can't trigger a single one with 10+ minutes of scanning, so it sounds pretty good.

If you want, you may also remove the -Wno-deprecated-declarations flag from configure, you'll see that a few gtk funcs are deprecated.

Wow! Ca. 4000 lines of warning output. With the later commits I've got it down to 1200 lines by now, still some more to go.

Oops, I didn't remember there were so many! That's probably why I silenced them and focused on fixing all the other warnings that came with the upstream version... Thanks for making the codebase better!

About the "raw editor" you referenced in an earlier comment, it's actually hidden, you have to start the program with the --debug option. This option is a misnomer really, it should have been called --expert or --advanced or --enable-dangerous-commands-and-dont-blame-the-dev-if-you-destroy-your-data. On the GUI, I think the only change is the "raw editor" in the "tools" menu. On the CLI, it enables dozens of hidden command-line parameters that were previously undocumented but that I added in the --help of this version, as long as you say --debug --help.

In the next few days I'll try to test the GUI more extensively (i.e. creating all ECC types, repairing all types, etc.). I might just have stumbled upon the somehow random segfaults you mentioned:

realloc(): invalid next size
*** BUG ***
In pixman_region_union_o: The expression region->data->numRects <= region->data->size was false
Set a breakpoint on '_pixman_log_error' to debug

*** BUG ***
In pixman_region_union_o: The expression region->data->numRects <= region->data->size was false
Set a breakpoint on '_pixman_log_error' to debug

Aborted (core dumped)

But I wasn't running under gdb... I'll try to reproduce that better.

(sorry about the PR close/open, just a misclick)

[pitdicker]

About the "raw editor" you referenced in an earlier comment, it's actually hidden, you have to start the program with the --debug option.

Ah, cool. The dialog seems quite happy to read unitialized memory or segfault if you don't have any raw sectors (on the main branch).
I wish I could play with it a bit more. What kind of drives can return raw sectors? I have an LG BH-16NS58, LG WH16NS60 (or whichever firmware suits me, sometimes ASUS BW-16D1HT), LG GGW-H20L, and HP DH-16AESH and they don't seem to support it.

[speed47]

Ah, cool. The dialog seems quite happy to read unitialized memory or segfault if you don't have any raw sectors (on the main branch).

I suspect the upstream dev was the main user of this dialog!

I wish I could play with it a bit more. What kind of drives can return raw sectors? I have an LG BH-16NS58, LG WH16NS60 (or whichever firmware suits me, sometimes ASUS BW-16D1HT), LG GGW-H20L, and HP DH-16AESH and they don't seem to support it.

Well, I have an iHAS324 F CL8N, and a WH16NS58 with the famous vinpower firmware 1.V5 (which supports PIF/POF PIE/POE C2 scans & co, it's actually a crossflashed WH16NS55). None of them seem to support raw reads either, with 0x20 or 0x21 read modes. I also have an external USB one, but there's zero chance it does support it either. So, I crafted a fake file which we can use to test this dialog, it has 10 sectors filled with random data, with the header that dvdisaster expects. I suspect drives supporting raw reads are now very rare, so, well, I guess the file will have to do the trick!

10rawrandomsectors.zip

[pitdicker]

Sorry for the little communication. Is PR is getting into shape, but is not quite there yet. The warnings for deprecated items in gtk3 are mostly taken care of, mostly a few icon-related ones are left.

What do you think about splitting this up to make it more manageable? Then I'll make a separate PR with all the preparations that are not drawing-related.

One other question: We currently have a preference to switch the Ok/Cancel button order of dialogs. That does not seem like something people get exited about, and uses deprecated functions (and has become impossible for the file picker in gtk3. Remove it?

[speed47]

Sorry for the little communication. Is PR is getting into shape, but is not quite there yet. The warnings for deprecated items in gtk3 are mostly taken care of, mostly a few icon-related ones are left.

No worries, take the time you need!

What do you think about splitting this up to make it more manageable? Then I'll make a separate PR with all the preparations that are not drawing-related.

This is probably a good idea. In any case, I'll NOT squash the PR, keeping all your commits is clearly better for future readability of the changes you made. Obviously this will also help a lot my review. Maybe we need a gtk3 branch (forked from the main branch) that you would target with several PRs, then when we're all done, just merge gtk3 into main?
This way there wouldn't be any moment where the main branch is somewhere between gtk2 and gtk3, and doesn't build properly?

One other question: We currently have a preference to switch the Ok/Cancel button order of dialogs. That does not seem like something people get exited about, and uses deprecated functions (and has become impossible for the file picker in gtk3. Remove it?

Yes.

Before your changes, I was trying to limit the number of changes I made to ease rebasing over potential future upstream commits, but even if upstream development shortly resumed in 2021 (mainly upstreaming some of my changes), nothing happened since, so I think the balance between "keeping the changes minimal to ease rebasing" versus "making the necessary changes to ease future maintenance" now clearly tilts over the second option. If some commits appear upstream in the future, I suspect they wouldn't be dramatic changes anyway, so I'll still be able to reapply them more "manually" than just using patch.

This is also the reason why I didn't use a C formatting tool to standardize the spaces/tabs (you probably noticed there's a mix of both) and the somehow exotic indentations. I'll probably end up doing that sometime in the future as "keeping the diff with upstream minimal" is no longer a thing with your PR porting the codebase to gtk3.

[pitdicker]

Great, I'll open a first PR tomorrow. I think just a gtk3 branch is not really needed because except for the the commit to switch to gtk3 and the one after that everything is incremental and builds and runs like it should.

[pitdicker]

And I've noticed the indentation 😄. My editor doesn't like it and works against me.

[speed47]

I've spent way too many hours this weekend into making the AppImage work under gtk3. I also fixed a few related shenanigans here and there, and hopefully also fixed #92 in the process. I had to heavily intervene into the AppImage bundling procedure, even going as far as hex-patching the gio lib has it has hardcoded paths that point to the host and it can't be changed at runtime.

Anyway, I have something that works quite nicely on top of this PR, tested under Ubuntu 18.04, 20.04, 22.04 and 24.04, so I'm quite confident this AppImage version will be quite resilient. I also added a continous build version that will push a pre-release with pre-built binaries under all OSes each time a new commit is pushed to master. Once you have your first PR up and after it's merged, I'll rebase my own PR on top of it, merge it, and you should be able to rebase on top of that one. I'm not expecting conflicts as I'm almost not touching the code itself.

[pitdicker]

EDIT: the code changes are minimal so they shouldn't conflict, I merged them. We now have prebuilt binaries for the main branch, for commits between official releases https://github.com/speed47/dvdisaster/releases/tag/latest

Nice!

This should finally be ready. To add some comments to the commits:

• Convert gtk_(h|v)box_new to gtk_box_new, Convert gtk_(h|v)separator_new to gtk_separator_new and Convert gtk_hscale_new_with_range to gtk_scale_new_with_range are just mechanical changes.
• The deprecated gtk_misc_set_alignment was used to set both the x and y alignment of labels. By default they are centered horizontally and vertically. But if the height of the container is determined by a label vertical alignment doesn't matter. I added one commit that does a 1:1 translation to the new gtk functions, and added another commit to remove the vertical alignmnets that where useless.
• As a drive-by fix with the label alignments the status bar label is now vertically aligned in the center.
• gtk_misc_set_padding should be replaced with four gtk_widget_set_margin_* functions. But in our cases only the padding to the left matters, so I left out the rest.
• Converting from GtkTable to GtkGrid was some work. I used screenshots to compare the before and after. The layout is the same of in a small number of cases alignment is a bit improved.
• While working on the icons I notices dvdisaster aborts when pressing undo in the RS2 preference tab when there is no media in the drive. As a fix I removed the media size check when pressing undo (which was ignored anyway).
• For the icons I found the original svg versions of the gtk icons after all. So the interface should not look any different, except it also looks good on hi-dpi monitors. Like before the icons are compiled into the binary. Apparently gresources are what we should use (all very new to me).
• Stock buttons are deprecated. In the little help dialogs and the preference dialogs I made a custom button with the close icon and label as before. A couple of other buttons have become text only.
• As it turns out gtk3 is not the only thing that does deprecations 😆. Don't use deprecated kIOMasterPortDefault is for a deprecated constant on MacOS. I pretty much just copied the solution from libusb/hidapi@a4ae913.

[speed47]

Thanks for the commits and the detailed explanation.
The conversion to GtkGrid indeed looks like it took some work!
From your screenshot it looks like dvdisaster will look sharp on recent OSes/monitors, that's great.

By reviewing the commits I'm just wondering whether

+    gtk_icon_theme_add_resource_path(gtk_icon_theme_get_default(), "/dvdisaster/");

Will work on the AppImage build, because as gtk has clearly not been engineered to work well with AppImage (or any notion of a "portable app not referencing the hosts libraries"), the approach I took was to include most things in the AppImage except the theme, as any sane distro seems to have a default one, even distros that are not shipping with Gnome. The "default theme" is then taken from the host transparently. When attempting to include themes in the image, I ran into a lot of weirdness and differing behaviors between distros, so I've taken the "the host always seems to have a working theme" route. I'm pushing your branch to the dev branch so that binaries get built and let's see how it turns out.

EDIT: I see now that the /dvdisaster/ above is a GResource path referencing the icons in icons/icons.gresource.xml. However, for some reason, I don't have the icons either in the AppImage version, the Windows version, nor if I just compile the Linux version myself and run it. All the icons default to the, I suppose, "default not-found icon". If you checkout your own branch somewhere else (to ensure there's no untracked file), does it work on your side? Or am I missing something?

[pitdicker]

EDIT: I see now that the /dvdisaster/ above is a GResource path referencing the icons in icons/icons.gresource.xml. However, for some reason, I don't have the icons either in the AppImage version, the Windows version, nor if I just compile the Linux version myself and run it.

Oops, interesting. On first run ./configure generates a makefile without inline-icons.c, and on second run a makefile with inline-icons.c. Why o why...

[pitdicker]

I don't really know how to properly work with configure. The last commit should fix the build problems. Do you know a better way?

[speed47]

Edit: This is not it. I'll investigate some more

Thanks, hopefully it'll be a minor quirk, one from the "oh, dammit!" quirks category.

On a somehow unrelated note, as we're now very close to complete the switch to gtk3, and given the amount of work we're talking about, would you be ok with me referencing you in the https://github.com/speed47/dvdisaster/blob/master/CREDITS.en file? I never added my own name (I'm not that narcissistic), at some point upstream did, and as their return is unknown, I feel like this should be done. I'm asking because I don't want to add your name without having your permission first, so, just tell me whether you would NOT have your name added to it. :)

[pitdicker]

O, thank you 😄. And for being patient above all. I don't mind either way, I just code for hobby and not for any recognition.

[pitdicker]

By reviewing the commits I'm just wondering whether

+    gtk_icon_theme_add_resource_path(gtk_icon_theme_get_default(), "/dvdisaster/");

Will work on the AppImage build, because as gtk has clearly not been engineered to work well with AppImage

You already made an edit, but just in case: I choose to go with the gresources route because it should work like before with an appimage; no extra files that need to be included.

[speed47]

I don't really know how to properly work with configure. The last commit should fix the build problems. Do you know a better way?

The tricky part is that you had to rename inlined-icons.h to inlined-icons.c, so indeed it messes with the configure script and the GNUmakefile.template. I'll try to find something better that works without requiring to touch/rm the files in configure. Maybe moving the generated inlined icons out of src/, because it seems weird/wrong to have generated files there (I know this was already the case with the inlined-icons.h file!).

Your fix seems to do the trick indeed, pushing to dev to get the AppImage build.

[speed47]

So, I have something that is somehow cleaner, or at least that doesn't require to modify the files during the configure, you can cherry-pick this commit on top of yours (and maybe squash them), it should apply cleanly: 19946f8

I've rebuilt binaries with this change, it seems to work even when swapping between "gui" and "cli" during the configure.

I've noticed however that the "read" button doesn't appear correctly, it seems that you named the file (and the resource) read-symbolic but referenced it in the code by read. Is there any reason? I've made this modification on top of the other commit that seems to fix the issue: 7711df0 . But if you avoided "read" as a name for a reason, maybe the proper change is to rename the "read" reference to "read-symbolic" instead.

The Windows version doesn't show any icons either, even with both these fixes, but it seems to be a Windows thing. I found some references online indicating that the icons are searched in the current directory (different pathnames are tried), so maybe we'll need to include the icons in some way in the Windows zip, but it's non-blocking for this PR.

[pitdicker]

So, I have something that is somehow cleaner, or at least that doesn't require to modify the files during the configure, you can cherry-pick this commit on top of yours (and maybe squash them), it should apply cleanly: 19946f8

I've rebuilt binaries with this change, it seems to work even when swapping between "gui" and "cli" during the configure.

Thank you, great!

I've noticed however that the "read" button doesn't appear correctly, it seems that you named the file (and the resource) read-symbolic but referenced it in the code by read. Is there any reason?

As a little trick I made this one a symbolic icon, following the documentation in https://developer.gnome.org/documentation/tutorials/themed-icons.html#symbolic-icons. In a dark theme the text will turn white. Can you tell me in what way it doesn't appear correctly?

The Windows version doesn't show any icons either, even with both these fixes, but it seems to be a Windows thing.

Strange. I am not sure what to do, it works on my Windows setup. Because the icons are compiled in paths should not matter.

[speed47]

I've noticed however that the "read" button doesn't appear correctly, it seems that you named the file (and the resource) read-symbolic but referenced it in the code by read. Is there any reason?

As a little trick I made this one a symbolic icon, following the documentation in developer.gnome.org/documentation/tutorials/themed-icons.html#symbolic-icons. In a dark theme the text will turn white. Can you tell me in what way it doesn't appear correctly?

Ah, interesting! I'll try that again to have something that can be reproduced (also, see below).

The Windows version doesn't show any icons either, even with both these fixes, but it seems to be a Windows thing.

Strange. I am not sure what to do, it works on my Windows setup. Because the icons are compiled in paths should not matter.

Well, I've gone into the rabbit hole of icon themes under gtk3, such as using an index.theme, using the freedesktop-approved directory structure to put the icons such as (scalable/actions) or it's silently ignored and taken as a "directory of icons" and NOT an "icon theme directory", explicitly loading the gresources from the ELF section before attempting to reference data in it, etc. more on that stuff in a later comment if we end up having to keep these changes. I also noticed that I had to dynamically add the proper pixbuf loaders (including svg for the new icons) to the Windows build, or it would silently default to the infamous "missing icon" for all icons. I think I'm close to something that works. When this is the case, I'll carefully undo some changes I made (such as the symbolic icon above) and see how it behaves under both Linux and Windows.