D5034: Add support for x-gvfs style options in fstab

[Kai Uwe Broulik]

broulik abandoned this revision.
broulik added a comment.


  Superseded by https://phabricator.kde.org/D7774

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D5034

To: broulik, #plasma, dhaumann, dfaure
Cc: bruns, dhaumann, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D5034: Add support for x-gvfs style options in fstab

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> fstabstorageaccess.cpp:48
>  
> +const QStringList  = FstabHandling::options(device->device());
> +// GVFS by default doesn't show devices outside of /media, $HOME (and 
> some other locations)

Another useless use of const QList&, FstabHandling returns a detached list.
Even if it were not detached, contains() would not detach.

> fstabstorageaccess.cpp:52
> +// only x-gvfs-hide is honored here.
> +m_isIgnored = options.contains(QLatin1String("comment=x-gvfs-hide"));
> +

According to the linked udisk2 documentation, this should be a plain 
"x-gvfs-hide", scrap the "comment=".

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D5034

To: broulik, #plasma, dhaumann, dfaure
Cc: bruns, dhaumann, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas

D5034: Add support for x-gvfs style options in fstab

[Dominik Haumann]

dhaumann added a comment.


  @broulik Ping. Is there anything that keeps you from submitting this?

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D5034

To: broulik, #plasma, dhaumann, dfaure
Cc: bruns, dhaumann, plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas

D5034: Add support for x-gvfs style options in fstab

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> fstabdevice.cpp:47
>  
> -m_description = m_vendor + " on " + m_product;
> +const QStringList  = 
> FstabHandling::options(m_device).filter("x-gvfs-");
> +

Why QStringList& instead of QStringList?

> fstabdevice.cpp:51
> +if (option.startsWith(QLatin1String("x-gvfs-name="))) {
> +const QStringRef  = 
> option.midRef(option.indexOf(QLatin1String("="), 11) + 1);
> +m_description = QUrl::fromPercentEncoding(encoded.toLatin1());

Useless &
use indexOf('=', 11) (i.e. QChar)

> fstabdevice.cpp:54
> +} else if (option.startsWith(QLatin1String("x-gvfs-icon="))) {
> +const QStringRef  = 
> option.midRef(option.indexOf(QLatin1String("="), 11) + 1);
> +m_iconName = QUrl::fromPercentEncoding(encoded.toLatin1());

use indexOf('=', 11)

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D5034

To: broulik, #plasma, dfaure, dhaumann
Cc: bruns, dhaumann, plasma-devel, #frameworks, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D5034: Add support for x-gvfs style options in fstab

[Kai Uwe Broulik]

broulik added a comment.


  If no-one objects I'll push this after the next frameworks release

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D5034

To: broulik, #plasma, dfaure, dhaumann
Cc: dhaumann, plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D5034: Add support for x-gvfs style options in fstab

[Dominik Haumann]

dhaumann accepted this revision.
dhaumann added a comment.
This revision is now accepted and ready to land.


  The patch looks good to me (without testing this locally here). I think the 
one QStringRef can be turned by to QStringList, since you finally need the 
QString anyways it seems...

INLINE COMMENTS

> fstabhandling.cpp:152
> +for (const QStringRef  : options) {
> +globalFstabCache->m_fstabOptionsCache.insert(device, 
> option.toString());
> +}

Well ok, given you need to convert the string to QString finally anyways, you 
can keep the QStringList above (line 148).

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D5034

To: broulik, #plasma, dfaure, dhaumann
Cc: dhaumann, plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D5034: Add support for x-gvfs style options in fstab

[Kai Uwe Broulik]

broulik updated this revision to Diff 12457.
broulik added a comment.


  - Cache mountOptions or else it falls out of scope and QStringRef crashes 
(now I know why I should cache it :D)

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5034?vs=12456=12457

REVISION DETAIL
  https://phabricator.kde.org/D5034

AFFECTED FILES
  src/solid/devices/backends/fstab/fstabdevice.cpp
  src/solid/devices/backends/fstab/fstabdevice.h
  src/solid/devices/backends/fstab/fstabhandling.cpp
  src/solid/devices/backends/fstab/fstabhandling.h
  src/solid/devices/backends/fstab/fstabstorageaccess.cpp
  src/solid/devices/backends/fstab/fstabstorageaccess.h

To: broulik, #plasma, dfaure
Cc: dhaumann, plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D5034: Add support for x-gvfs style options in fstab

[Kai Uwe Broulik]

broulik updated this revision to Diff 12456.
broulik edited the summary of this revision.
broulik added a comment.


  - Use QStringRef
  - Drop swap of vendor/product, this will be done in a separate patch

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5034?vs=12435=12456

REVISION DETAIL
  https://phabricator.kde.org/D5034

AFFECTED FILES
  src/solid/devices/backends/fstab/fstabdevice.cpp
  src/solid/devices/backends/fstab/fstabdevice.h
  src/solid/devices/backends/fstab/fstabhandling.cpp
  src/solid/devices/backends/fstab/fstabhandling.h
  src/solid/devices/backends/fstab/fstabstorageaccess.cpp
  src/solid/devices/backends/fstab/fstabstorageaccess.h

To: broulik, #plasma, dfaure
Cc: dhaumann, plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D5034: Add support for x-gvfs style options in fstab

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> broulik wrote in fstabdevice.cpp:40-41
> I wondered that too, that's in the original patch, though, didn't really look 
> into where this will end up, though :D

Just checked, this way round it makes more sense.

The vendor will be the host name and product the path. However, I'll split that 
into a separate patch then.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D5034

To: broulik, #plasma, dfaure
Cc: dhaumann, plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D5034: Add support for x-gvfs style options in fstab

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> dhaumann wrote in fstabdevice.cpp:40-41
> Did you intentionally switch vendor and product?

I wondered that too, that's in the original patch, though, didn't really look 
into where this will end up, though :D

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D5034

To: broulik, #plasma, dfaure
Cc: dhaumann, plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D5034: Add support for x-gvfs style options in fstab

[Dominik Haumann]

dhaumann added a comment.


  Just some minor comments, besides that, looks sane.

INLINE COMMENTS

> fstabdevice.cpp:40-41
> +if (m_device.startsWith(QLatin1String("//"))) {
> +m_vendor = m_device.mid(2, m_device.indexOf(QLatin1String("/"), 2) - 
> 2);
> +m_product = m_device.mid(m_device.indexOf(QLatin1String("/", 2)) + 
> 1);
>  } else {

Did you intentionally switch vendor and product?

> fstabdevice.cpp:51
> +if (option.startsWith(QLatin1String("x-gvfs-name="))) {
> +const QString encoded = 
> option.mid(option.indexOf(QLatin1String("="), 11) + 1);
> +m_description = QUrl::fromPercentEncoding(encoded.toLatin1());

QStringRef through midRef()

> fstabdevice.cpp:54
> +} else if (option.startsWith(QLatin1String("x-gvfs-icon="))) {
> +const QString encoded = 
> option.mid(option.indexOf(QLatin1String("="), 11) + 1);
> +m_iconName = QUrl::fromPercentEncoding(encoded.toLatin1());

likewise, use QStringRef

> fstabhandling.cpp:146
>  const QString mountpoint = QFile::decodeName(fe->mnt_dir);
> +const QStringList options = 
> QFile::decodeName(fe->mnt_opts).split(QLatin1Char(','), 
> QString::SkipEmptyParts);
>  

If you save the decodedName, you could also call splitRef() to avoid more 
allocations.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D5034

To: broulik, #plasma, dfaure
Cc: dhaumann, plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D5034: Add support for x-gvfs style options in fstab

[Kai Uwe Broulik]

broulik created this revision.
Restricted Application added projects: Plasma, Frameworks.
Restricted Application added subscribers: Frameworks, plasma-devel.

REVISION SUMMARY
  These fstab options allows an administrator to specify names and icons 
intended for the user, shown in a GUI.
  
  For details, see 
https://git.gnome.org/browse/gvfs/tree/monitor/udisks2/what-is-shown.txt

TEST PLAN
  This is a frameworks port and cleanup of 
https://git.reviewboard.kde.org/r/113587
  
  Placed the following in `/etc/fstab`
  
hostname:/ /mnt/ nfs4 
defaults,_netdev,user,rw,exec,comment=x-gvfs-show,x-gvfs-name=Test%20Folder,x-gvfs-icon=folder-home,timeo=14,noatime
 0 0
  
  Got the following entry:
  F2800877: Screenshot_20170313_152729.png 

  Also verified that `comment=x-gvfs-hide` hides the entry. I did not implement 
`x-gvfs-show` as we always show all devices no matter where they are, so the 
override to forcefully show it is of no use to us.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D5034

AFFECTED FILES
  src/solid/devices/backends/fstab/fstabdevice.cpp
  src/solid/devices/backends/fstab/fstabdevice.h
  src/solid/devices/backends/fstab/fstabhandling.cpp
  src/solid/devices/backends/fstab/fstabhandling.h
  src/solid/devices/backends/fstab/fstabstorageaccess.cpp
  src/solid/devices/backends/fstab/fstabstorageaccess.h

To: broulik, #plasma, dfaure
Cc: plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol