| dfaure |
| Frameworks |
This patch adds the support for templates in the XDG_TEMPLATES_DIR location which points in default to:
[en] /home/user/templates
[de] /home/user/Vorlagen
As is all templates have to be a .desktop file maybe a second patch will follow changing this but it seems to be tricky without breaking something.
This is a first step in resolving bug: #191632
Add new valid .desktop file to your template folder -> shows up in the menu
| Lint Skipped |
| Unit Tests Skipped |
Pretty cool! How about moving the custom ones to the top below Folder? Imho if the user went through the trouble of adding custom templates, he or she is most likely to use them most often? Perhaps also add a separator line?
@broulik Thanks :-)
My idea was to add a submenu "own templates" to the menu but this would greatly exceed my knowledge. At the moment this is more try, error and debgugging, learning by doing ^^
Will try to improve this patch further and see what I can do here because my main goal is that we can use normal files as templates again ;-)
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:
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?
| src/filewidgets/knewfilemenu.cpp | ||
|---|---|---|
| 896 ↗ | (On Diff #26702) | All your new code should be in #ifdef Q_OS_UNIX, it is not applicable on e.g. Windows. |
| 899 ↗ | (On Diff #26702) | remove the exists() check, open will fail if it doesn't exist, anyway, so we might as well save one syscall. |
| 904 ↗ | (On Diff #26702) | the 2nd arg isn't necessary, just do line.mid(19) |
| 905 ↗ | (On Diff #26702) | QStringLiteral("$HOME/") No need to use QStandardPaths to get the home dir (that's... historical), use QDir::homePath() directly. |
| 910 ↗ | (On Diff #26702) | 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. |
| 915 ↗ | (On Diff #26702) | not needed, the destructor closes. |
@dfaure Thank you for your answer :-)
As I am still at the beginnings I am really grateful for such constructive and valuable feedback like yours it helps me to become better and improve my code.
Regarding your points:
First of all I agree that a nice name istead of a simple file name is much better. On the other side there can only be one template with this name and if this one is not choosen very well then it's hard to guess that this template is for but I think this shouldn't be the case so often.
When I started my journey with Linux it begin with Gnome2, over the time I moved to Gnome3 but had more and more issues. After I read an article about the early 5.0 version from Plasma and saw some screenshots and felt like: You have to use this, it's beautiful! (I never liked how oxygen looked like and as teen and linux noob in the past I didn't even know how to change the look and so never took a deeper look into the world of KDE ... what a shame) and so I switched to KDE and Plasma :)
To shorten the things a little bit ... I liked the way templates are handled in Gnome, you just put your custom files into the folder and they appear in the menu, even with the folder hirarchy if you had one and this is something I missed a lot in the beginning. Until a few days I even don't know that a customization was possible at all ^^
I was thinking about a display name which gets the name for the mimetipe and adds the file name to it, if this is possible to get something like: "HTML File: StartPage.html"
Then you have reservations about file support in KDE dirs and that's okay I think. This patch adds no support for "normal" files and I don't plan to add it to this revision. Maybe a second patch will follow which enables this feature again, but I didn't achive anything that worked correct until now. But lets move forward, should I be able to make this happen I could limit this support to the local template folder which does make a lot of sense for me too.
The reason I added this to the installedTemplates was that I can then easiely use the KDirWatch and new files would be detected automatically which is important because otherwise the whole function does not make sense for me.
At least I don't know any and was not able to find something. If there would be something it would be great to addapt it, because a great workflow would be:
Drag'n Drop a file (or create a new one) to the template dir
A dialog would open where the user can enter a nice name, maybe a comment also and after that the local file would be moved to an .source dir and the .desktop file would be greated on the fly like you said
This would be awesome :)
I will work on your improvements regarding the code. Most of them I didn't know I took a look and similar code, searched in the documentation of QT and just wanted to make it safe and clean but great if I can get rid of most of them which will make it even simpler ;-)
Just one note for this point:
the 2nd arg isn't necessary, just do line.mid(19)
I can't get rid of these because a normal entry in the user-dirs.dirs looks like: XDG_TEMPLATES_DIR="$HOME/Templates" so I have to remove the " before and at the end of the path.
Sorry for the wall of text ^^
| src/filewidgets/knewfilemenu.cpp | ||
|---|---|---|
| 904 ↗ | (On Diff #26702) | It is because we need to get rid of the " before and at the end of the path. |
Hello, sorry for the delay in my review, I failed to react to the notification and then it scrolled out of view.... :/
| src/filewidgets/knewfilemenu.cpp | ||
|---|---|---|
| 642 | That's not a (user-visible) text, that's a path, please rename the variable to lastTemplatePath or something like that. | |
| 902 | typo: receive | |
| 977 | can you swap the conditions so that this code still is ordered 0, 1, 2, 3? Makes it more readable (since it will match the resulting menu order) But actually, shouldn't we still have New Folder and New Text File at the top before everything else? | |
| 904 ↗ | (On Diff #26702) | Ah I see, OK. |
No problem David, it's okay :)
I will work on your suggestions and update this revision accordingly.
But actually, shouldn't we still have New Folder and New Text File at the top before everything else?
Kai mentioned he wants them directly underneath the "new folder" so I changed it.
Where should I place them finally ?
I think it woulnd't be a good thing to mix the system templates directly with the user templates.
So I can place the user templates on the first position underneath the folder like it is now, or underneath all system templates (like before).
Personally I would be okay with both options. When you say I should revert it again I will do this. Should I keep the seperator then?
Ah OK, let's do that then. I was just wondering if it was an intentional change or not.
@mmustac very nice! Should we consider 191632 to be resolved, or wait until you're implemented support for plain old non-.desktop files in ~/Templates?
I would not close the bug yet. Unfortunately I am busy at the moment so I can't tell you when I will find to take a deeper look at the main problem from the bug:
Although we have support for the template directory now, it is still very difficult to display your own templates in the menu. At least I will try to solve this problem without breaking some of those bugfixes which disabled this feature in the past.
