Add handling of fuseiso filesystem type.
When a fuseiso filesystem is mounted it will be shown as a optical data
media.
Details
Details
Diff Detail
Diff Detail
- Repository
- R245 Solid
- Branch
- add_handling_of_fuseiso (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 17991 Build 18009: arc lint + arc unit
| src/solid/devices/backends/fstab/fstabdevice.cpp | ||
|---|---|---|
| 126 | I don't know if this is the best icon to use, but I think it should be something that the user would associate with a CD since an ISO is essentially a CD image. | |
Comment Actions
I had iso images shown as loop devices through udisks already, why is this needed? Patch looks fine, just curious.
Comment Actions
I think udisks only shows the disk image if it is using a kernel device, in your case a loop device.
Comment Actions
Right. +1
| src/solid/devices/backends/fstab/fstabdevice.cpp | ||
|---|---|---|
| 115 | Can we maybe give it a better description then? For loop devices we show the name of the backing file as description. | |
| src/solid/devices/backends/fstab/fstabdevice.cpp | ||
|---|---|---|
| 115 | Hmm, hard to tell without some context or knowing what the original string is. | |
| src/solid/devices/backends/fstab/fstabdevice.cpp | ||
|---|---|---|
| 115 | This is what I have in mounts: fuseiso on /home/dha/mnt1 type fuse.fuseiso (rw,nosuid,nodev,relatime,user_id=1000,group_id=1000) so there just isn't a whole lot of information - we don't even have the iso filename :/ | |
| src/solid/devices/backends/fstab/fstabdevice.cpp | ||
|---|---|---|
| 115 | By default fuseiso seems to maintain also a file "~/.mtab.fuseiso", there may be more information | |
| src/solid/devices/backends/fstab/fstabdevice.cpp | ||
|---|---|---|
| 115 | Yes it does :D After mounting an iso I have the following contents: /home/dha/test.iso /home/dha/mnt1 fuseiso defaults 0 0 So at least we should be able to dig out the iso filename. So could a better description be something like: test.iso on /home/dha/mnt1 or test.iso (/home/dha/mnt1) or maybe some mention that this is a fuseiso mount, i.e. test.iso fuseiso mounted on /home/dha/mnt1 I don't know if that becomes way too long | |
Comment Actions
test.iso (/home/dha/mnt1) is probably okay, though a bit long and will cause the Places panel in Dolphin to get a horizontal scrollbar. Maybe just test.iso, even. I'm not sure how useful it is to have the path listed in the title considering that this information is available in its Properties window.
Comment Actions
I have been resurrecting this patch again :) and have run into an issue I need some guidance on. To be able to parse the ~/.mtab.fuseiso file I would like to use the KMountPoint class, but this class currently resides in KIO which Solid doesn't depend on. But, KIO actually depends on Solid so would it be an option to move this class from KIO to Solid?
Comment Actions
What do you gain by using KMountPoints? Solid already has code for parsing fstab style files.
Comment Actions
Hi @bruns , no you are right that I should be able to do with the code that is in Solid already. It just needs to be updated to be able parse an arbitrary mtab file. But anyhow, the fstab/mtab code in Solid seems kind of duplicated with the code in KIO, so we might consolidate at some point.
But let me look into the mtab code in Solid and see if I can make it work there :)
Comment Actions
Ok, I have been playing around with the code and using getmntent for parsing the ~/.mtab.fuseiso file, but the getmntent function doesn't give me the first field, which is the path to the iso (this is the information I am looking for). So currently I am looking at just reading in the file, splitting the space separated lines and extracting the two first fields. If anyone has ideas to do this in a smarter way, please let me know ;) I will post an updated review once I have some working code.
Comment Actions
Currently you have the 'unmount' action if you right click on the device in dolphin, but it cannot unmount, so should we hide it? Or should we fix it so that it can actually unmount?
| src/solid/devices/backends/fstab/fstabdevice.cpp | ||
|---|---|---|
| 56 | This implements a crude parser of the fuseiso mtab file. I can't use the regular getmnt suite of functions to retrieve the path to the iso file, so that is why I implemented this :/ | |
| src/solid/devices/backends/fstab/fstabdevice.cpp | ||
|---|---|---|
| 56 | fuseiso itself uses setmntent/addmntent/endmntent, so I see no reason getmntent should not work. https://sourceforge.net/p/fuseiso/code/HEAD/tree/trunk/src/fuseiso.c#l112 | |
| src/solid/devices/backends/fstab/fstabdevice.cpp | ||
|---|---|---|
| 56 | Yes i know :) But when I played around with the existing code in fstabhandling.cpp for reading out the fsname it kept being empty. But now I just wrote my own little test program that dumps every field in the mntent struct and there the fsname contains the full path to the iso file. So, I must have screwed up when fiddling with the fstabhandling.cpp code, let me give it another go, I think it would be much nicer to reuse that code then to have our own parser :) | |
Comment Actions
@bruns - I have now refactored the patch so that it uses the getmntent functions for parsing the mtab file, so I think this patch is pretty much ready for a serious review ;)
Also - please take a look at the strings that the end user sees, I am not sure if they are optimal.
| src/solid/devices/backends/fstab/fstabhandling.cpp | ||
|---|---|---|
| 301 | Please keep these functions at their current position, just adds unnecessary noise in the diff. | |
| src/solid/devices/backends/fstab/fstabhandling.cpp | ||
|---|---|---|
| 301 | Good point :) | |
Comment Actions
@bruns - ping :) I have updated this patch with the changes you requested, I hope you are ok with it now.
| src/solid/devices/backends/fstab/fstabhandling.cpp | ||
|---|---|---|
| 301 | The API is totally awkward. You end up with something significantly cleaner when you create a new class for it: QStringMultiHash m_mtabCache; QHash<QString, QString> m_mtabFstypeCache; bool m_mtabCacheValid; and make the parser a member function. Then instantiate the class for the fuseiso case and for the globalFstabCache. | |
| src/solid/devices/backends/fstab/fstabhandling.cpp | ||
|---|---|---|
| 301 | Yeah, I agree - it isn't the prettiest of code :D My main reasoning for doing it like this was to make the change as small as possible. But, let me take a stab at refactoring it a bit and push a new commit. | |
| src/solid/devices/backends/fstab/fstabdevice.cpp | ||
|---|---|---|
| 92 | That's incorrect concatenation of translated strings. | |
| src/solid/devices/backends/fstab/fstabdevice.cpp | ||
|---|---|---|
| 92 | yeah, should be m_description = i18nc("provide context here", %1 on %2, ShortenPath(isoFilePath), m_product); | |
Comment Actions
I have refactored the fstab handling to make supporting fuseiso _much_ simpler - so please take a look at D26600 - once that is merged I will push a new review of this patch.