D10363: [KIO] Add support for XDG_TEMPLATES_DIR in KNewFileMenu

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  Right, apart from the coding issues I found, the bigger issue is that this will not work, xdg templates are simple files I guess, not desktop files.
  
  In case you're wondering, the main reason I killed the support for normal files was:
  
  - we need translated names, which just isn't available with just a filename. But OK, in the user's dir, we can assume they understand their own filenames ;)
  - a "nice" display name is better than just a filename.
  
  I don't mind some kind of "xdg compatibility" with a lower feature set, but I wouldn't want this code to suddenly support normal files in the KDE dirs, it will just make people (e.g. distros who like to add their own templates) ignore those issues and go for solutions that don't support nice and translated display names.
  
  So maybe the code shouldn't be adding to the installedTemplates variable, but rather doing its own parsing of the xdg template dir, separately from everything else.
  
  Remind me though, do we have a GUI for users to add templates into the already-supported (non-xdg) user-specific template dir, creating the .desktop file for it on the fly?

INLINE COMMENTS

> knewfilemenu.cpp:896
> +    QStringList installedTemplates = { QStandardPaths::locateAll(QStandardPaths::GenericDataLocation, QStringLiteral("templates"), QStandardPaths::LocateDirectory) };
> +    // Qt does not provide as easy way to recieve the xdg dir for templates so we have to find it by our own
> +    QString xdgUserDirs = QStandardPaths::locate(QStandardPaths::ConfigLocation, QStringLiteral("user-dirs.dirs"), QStandardPaths::LocateFile);

All your new code should be in #ifdef Q_OS_UNIX, it is not applicable on e.g. Windows.

> knewfilemenu.cpp:899
> +    QFile xdgUserDirsFile(xdgUserDirs);
> +    if (xdgUserDirsFile.exists() && xdgUserDirsFile.open(QIODevice::ReadOnly | QIODevice::Text)) {
> +        QTextStream in(&xdgUserDirsFile);

remove the exists() check, open will fail if it doesn't exist, anyway, so we might as well save one syscall.

> knewfilemenu.cpp:904
> +            if (line.startsWith("XDG_TEMPLATES_DIR=")) {
> +                QString xdgTemplates = line.mid(19, line.size()-20);
> +                xdgTemplates.replace(QString("$HOME/"), QStandardPaths::locate(QStandardPaths::HomeLocation, QString(), QStandardPaths::LocateDirectory));

the 2nd arg isn't necessary, just do line.mid(19)

> knewfilemenu.cpp:905
> +                QString xdgTemplates = line.mid(19, line.size()-20);
> +                xdgTemplates.replace(QString("$HOME/"), QStandardPaths::locate(QStandardPaths::HomeLocation, QString(), QStandardPaths::LocateDirectory));
> +                QDir xdgTemplatesDir(xdgTemplates);

QStringLiteral("$HOME/")

No need to use QStandardPaths to get the home dir (that's... historical), use QDir::homePath() directly.

> knewfilemenu.cpp:910
> +                } else {
> +                    xdgTemplates = nullptr;
> +                }

wait, what? Interesting that this compiles, it's not the Qt way to clear a string (that would be .clear()). But just kill the line (and therefore the else), it's unnecessary, the variable is going out of scope anyway.

> knewfilemenu.cpp:915
> +        }
> +        xdgUserDirsFile.close();
> +    }

not needed, the destructor closes.

REPOSITORY
  R241 KIO

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

To: mmustac, #frameworks, dfaure
Cc: broulik, ngraham, michaelh
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180207/4a33c0f7/attachment-0001.html>