DriveSetup: add image menu

[dsjonny]

It would be useful if we can mount/unmount disk images from DriveSetup.

Patch has been added.

Using hrev46281 nightly anyboot.

[dsjonny]

Tested. Works fine.

[axeld]

Thanks for the patch! The basic idea is good, but the patch has a number of issues:

• There is unused stuff in it like write/save image menu items, whatever you thought they could do one day, unused things don't belong into a patch.
• The naming is strange "load" and "unload" is even more cryptic than register and unregister. Why not just "Mount image" and "Remove image"?
• It leaks the file panel.
• A few minor coding style issues (like '{' after case -- it goes to the next line there).

[dsjonny]

Replying to axeld:

Thanks for the patch! The basic idea is good, but the patch has a number of issues:

• There is unused stuff in it like write/save image menu items, whatever you thought they could do one day, unused things don't belong into a patch.
• The naming is strange "load" and "unload" is even more cryptic than register and unregister. Why not just "Mount image" and "Remove image"?
• It leaks the file panel.
• A few minor coding style issues (like '{' after case -- it goes to the next line there).

Thanks for checking. I will fix these.

With the save/load image the user would save the selected partition/disk (or it's part) to an image or copy it to another disk. It would be the similar as dd do. But I have no idea how to make it usable.

[dsjonny]

I have fixed the issues. I hope there will be no more problem.

[axeld]

Thanks for taking my comments into account so fast! However, I might have been a bit misleading: with "Mount image" I also implied that the image would actually be mounted. I would also remove the success alert, as you will see the image in your column list anyway.

Furthermore, the member variables should be named as their contents. If it would be called "Register image" that name would be fine. If it is "Mount image", though, it should also be fMountImageMenuItem.

And on second thought, I think "image" is not even expressive enough. I would actually label the menu items "Mount disk image", and "Remove disk image". BTW, does anybody think that just registering an image would be a worthwhile action? If so, the menu should probably look like this:

   Mount disk image
   ---------------------
   Register disk image
   Unregister disk image

Finally, the mentioned coding style problem is still there.

[dsjonny]

I will fix the naming also and the menu's text. Your suggestions are better. But maybe the "mount" is not so clear, because there is also a mount item in the disk menu. And in this case the mount is not the mount, it's just add the disk image to able to mount. Maybe the "Disk image" would be enught for the menu's text.

All alerts are from the diskimage code, but yes, the success alert is not neccessary.

Can you tell me exactly what is the coding style problem?

[dsjonny]

Fixed namings and texts.

[axeld]

Of course, and as I said, "Mount disk image" should actually not only register, but also mount the disk image -- hence the label :-)

To quote myself on the coding style problem: like '{' after case -- it goes to the next line there IOW you wrote:

case a: {
    break;
}

While it should be:

case a:
{
    break;
}

[axeld]

BTW, the patch looks almost good now, however, the file panel is still leaked.

[dsjonny]

Replying to axeld:

BTW, the patch looks almost good now, however, the file panel is still leaked.

Pff... So; what does it mean?

[axeld]

To leak means that you allocate it, but never free it. Each time you register an image, a file panel is allocated, but never freed. Furthermore, fOpen is not really a good name, it should rather be something like fOpenPanel.

BTW, the static functions should not be intermixed with the class methods but go above them.

Sorry for finding so many things to nitpick over, but we're trying hard to keep the code quality as high as possible in Haiku, and maybe you learn a bit on the way, too. In any case, thanks for your work! I hope you're still motivated to bring this patch into shape for committing. It's not far anymore :-)

[dsjonny]

Added 0001-Add-disk-image-menu-to-register-unregister-disk-images.patch patch file. Corrected the mentioned issues.

[stippi]

Closing the ticket, since a newer version of the patch, with additional features, is contained in #10134.