app-theme support #52
Conversation
appinfo/routes.php
Outdated
|
|
||
| ))); | ||
| $application->registerRoutes($this, [ |
There was a problem hiding this comment.
you can just return the array - no need to call registerRoutes
There was a problem hiding this comment.
thanks, didn't know that
lib/MailTemplate.php
Outdated
|
|
||
| use OC\Theme\Theme; | ||
| use OCA\TemplateEditor\Http\MailTemplateResponse; | ||
| use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; |
There was a problem hiding this comment.
no ocp for this exception?
There was a problem hiding this comment.
silly me, must have been in there from before, this exception isn't actually been thrown anywhere
lib/MailTemplate.php
Outdated
|
|
||
| namespace OCA\TemplateEditor; | ||
|
|
||
| use OC\Theme\Theme; |
There was a problem hiding this comment.
looks like we need a public api for this
lib/TemplateEditor.php
Outdated
| @@ -0,0 +1,80 @@ | |||
| <?php | |||
| /** | |||
| * Created by PhpStorm. | |||
There was a problem hiding this comment.
please use our standard header - thx
There was a problem hiding this comment.
damn PhpStorm and its autogenerated stuff...
|
@DeepDiver1975 please re-check! |
|
Increased version |
|
and set min version to 10.0.3 |
|
Since owncloud/core#27746 is merged, this one can be merged too if we get a positive review. @DeepDiver1975 @PVince81 |
|
a quick retest would be appreciated since this PR is older |
|
@phisch can you retest ? |
|
Somehow something with the DI-Container changed so the templateeditor could not resolve its dependencies anymore. Fix is here: owncloud/core#28925 |
|
There are some issues where the routes are not being loaded correctly in a setup of @michaelstingl. Trying to find out what is going wrong there atm. |
|
The list of themes is empty in @michaelstingl's setup because the themes have never been enabled. That means that there is no obvious way to get a list of all app themes if they have never been enabled before. Looking for a fix/workaround for that. |
|
This actually needs a fix in the core. A quick fix pr is here: owncloud/core#29110 But i am not sure if we can go that way or have to find a more elegant solution. |
|
But it doesn't make sense to display anything about the theme in this dropdown if the theme app was never enabled. We don't want to go fiddle with disabled apps. |
VicDeo
force-pushed
the
app-theme-support
branch
from
8677872
to
dfb0a03
Compare
…re-written some code so it is up to date, also used depencency injection, so we can add tests
…nnotation that isn't actually been thrown, fixed docblock, removed todo that is unnecessary
VicDeo
force-pushed
the
app-theme-support
branch
from
4fe32a9
to
7617f4b
Compare
|
@PVince81 Rebased, squashed a bit, minor cleanup is done, routing issue is fixed. So the theme should be enabled and only then it is possible edit email templates for this theme? It differs from the previous approach when we were able to edit templates for any theme from |
|
@VicDeo ah, so it was possible before, I didn't know that. If we consider the actual use case, I don't see a real use case for the ability to edit disabled themes. It's not like someone is switching themes every week or so... So I think the ability to only edit the currently active theme is alright. cc @pmaier1 |
|
@PVince81 Ok, only enabled app-themes are available now. As for non-app-themes they are all available as before. |
|
@DeepDiver1975 @PVince81 ready for review. |
|
Code looks good so far. Does the editor still works with old style themes stored in "/themes" ? I don't remember whether we still support non-app themes at this point. |
|
@PVince81 yes, old style themes are still supported here |
This adds app theme support. I also refreshed the code in general, using dependency injection, file naming and more.
This PR has a hard dependency on owncloud/core#27746
Fixes: owncloud/core#27628
Fixes #54