Opened 6 years ago

Closed 6 years ago

#10141 closed enhancement (duplicate)

DriveSetup: add image menu

Reported by: dsjonny Owned by: stippi
Priority: low Milestone: R1
Component: Applications/DriveSetup Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

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

Patch has been added.

Using hrev46281 nightly anyboot.

Attachments (2)

0001-Added-image-menu-to-mount-unmount-disk-images-from-D.patch (10.2 KB ) - added by dsjonny 6 years ago.
Fixed namings and texts.
0001-Add-disk-image-menu-to-register-unregister-disk-images.patch (9.3 KB ) - added by dsjonny 6 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 by dsjonny, 6 years ago

Has a Patch: set

comment:2 by dsjonny, 6 years ago

Tested. Works fine.

comment:3 by axeld, 6 years ago

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).

in reply to:  3 comment:4 by dsjonny, 6 years ago

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.

comment:5 by dsjonny, 6 years ago

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

comment:6 by axeld, 6 years ago

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.

comment:7 by dsjonny, 6 years ago

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?

by dsjonny, 6 years ago

Fixed namings and texts.

comment:8 by axeld, 6 years ago

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;
}

comment:9 by axeld, 6 years ago

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

in reply to:  9 comment:10 by dsjonny, 6 years ago

Replying to axeld:

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

Pff... So; what does it mean?

comment:11 by axeld, 6 years ago

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 :-)

comment:12 by dsjonny, 6 years ago

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

comment:13 by stippi, 6 years ago

Resolution: duplicate
Status: newclosed

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

Note: See TracTickets for help on using tickets.