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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20170313/ddaec103/attachment.html>