extended theme service by phisch · Pull Request #27746 · owncloud/core

phisch · 2017-04-26T07:17:28Z

Description

This adds the functionality to get a list of all available themes or search for one through the ThemeService.

Related Issue

Templateditor PR owncloud/templateeditor#52, which fixes #27628, has a hard dependency on this extension

Motivation and Context

The mail template editor needs to know which themes there are, and i don't want too much theme logic it its repository, this way we can keep it clean.

How Has This Been Tested?

Partially tested through unittests. Fully tested by hand.
The usual overwrites: css, js, img, templates, translations, mimetype icons all work for legacy and app themes, for subdir and normal deployments.

Tested the following overwrites:

app theme - regular deployment:

app theme - subdir deployment:

legacy theme - regular deployment:

legacy theme - sub dir deployment:

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

PVince81 · 2017-05-04T05:04:18Z

Is this done ? If yes, please set the "3 - To Review" label and summon reviewers.

phisch · 2017-05-05T08:59:38Z

Since the changes are used in an app, should i move this to the public api?

PVince81 · 2017-05-05T09:35:14Z

 		$this->setDefaultThemeDirectory($defaultThemeDirectory);
-		$this->createTheme($themeName);
+
+		if ($themeName === '' && $this->defaultThemeExists()) {


weird, why does Github show && in red now ?

Github had a syntax highlighting hickup. (<- just for the history)

PVince81 · 2017-05-05T09:48:48Z

Since the changes are used in an app, should i move this to the public api?

If some functions need to be called by apps, please make these public.

The code checker would complain when scanning the app if you're using private APIs

phisch · 2017-05-05T11:08:19Z

Ok, going to update.

DeepDiver1975

public api is required

PVince81 · 2017-05-15T09:39:44Z

@phisch any update ?

phisch · 2017-05-16T07:17:29Z

@DeepDiver1975 please give me feedback if this is okay.

Updating the TemplateEditor regardingly, and checking if this is being used somewhere else.

DeepDiver1975 · 2017-05-16T08:32:23Z

@DeepDiver1975 please give me feedback if this is okay.

OCP shall only hold interfaces - no classes. the idea here is to separate the public interface declaration from its implementation.

DeepDiver1975 · 2017-05-17T06:53:11Z

setting back to '2 - developing' because of missing proper public interfaces

PVince81 · 2017-05-17T06:54:31Z

Without this it will not be possible to theme the setup page as it was before, which is a regression.

Not sure how bad this is and whether it can wait for 10.0.2 @felixboehm ...

PVince81 · 2017-05-17T07:07:11Z

@phisch or is that only needed for the email template app?

DeepDiver1975 · 2017-05-18T14:09:20Z

 		$this->registerService('ThemeService', function ($c) {
 			return new ThemeService($this->getSystemConfig()->getValue('theme'));
 		});
+		$this->registerAlias('IThemeService', 'ThemeService');


Try 'OCP\Theme\IThemeService'

PVince81 · 2017-05-19T09:10:52Z

@phisch how much work is missing to have this working ?

phisch · 2017-05-19T09:24:32Z

@PVince81 ups, i just forgot to switch to review label again. It should be done.

PVince81 · 2017-05-19T09:26:54Z

It's still a huge thing, looks a bit risk to merge now.

PVince81 · 2017-05-19T09:30:33Z

moving to 10.0.2

PVince81 · 2017-07-04T12:19:05Z

@phisch phinisch this ?

phisch · 2017-07-12T11:41:54Z

@PVince81 this has been complete the whole time, we just did not want to merge because it was too close to a release.
@DeepDiver1975 can you re-review?

DeepDiver1975 · 2017-07-12T12:36:48Z

translation have been tested? php as well as js translations?

I don't see any related change in the code which could make it possible - see #28167 (comment)

@phisch

phisch · 2017-07-13T12:21:41Z

I tested the JS translations which could be affected by this. And yes they work as expected.
Mainly this PR helps other apps interacting with themes, especially getting information about available ones.

This should not affect existing behavior. And to make sure it doesn't i re-tested all the points mentioned in the first comment.

@DeepDiver1975

PVince81 · 2017-08-03T10:41:12Z

@phisch can you rebase to refresh CI ?

@DeepDiver1975 so this can be merged ?

…eme, refactored theme object so it can also set the web path through constructor, added setters to manipulate the current theme, removed unnecessary slash, added/modified tests

…locks, implemented interfaces

PVince81 · 2017-08-10T07:06:04Z

rebased

PVince81 · 2017-08-10T19:01:17Z

@DeepDiver1975 no feedback from you, assuming you don't have any comment to add

lock · 2019-08-02T21:02:42Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

phisch mentioned this pull request Apr 26, 2017

app-theme support owncloud/templateeditor#52

Merged

DeepDiver1975 force-pushed the extend-theme-service branch from ee58be4 to 74e6e41 Compare May 2, 2017 07:45

phisch added the 3 - To Review label May 5, 2017

phisch requested review from DeepDiver1975, IljaN and PVince81 May 5, 2017 08:58

PVince81 reviewed May 5, 2017

View reviewed changes

DeepDiver1975 added this to the 10.0.1 milestone May 12, 2017

DeepDiver1975 requested changes May 12, 2017

View reviewed changes

PVince81 added 2 - Developing and removed 3 - To Review labels May 15, 2017

phisch added 3 - To Review and removed 2 - Developing labels May 16, 2017

phisch self-assigned this May 16, 2017

DeepDiver1975 added 2 - Developing and removed 3 - To Review labels May 17, 2017

phisch force-pushed the extend-theme-service branch from cf2f8e4 to 74e6e41 Compare May 18, 2017 10:07

DeepDiver1975 reviewed May 18, 2017

View reviewed changes

phisch added the 3 - To Review label May 19, 2017

phisch removed the 2 - Developing label May 19, 2017

PVince81 modified the milestones: 10.0.2, 10.0.1 May 19, 2017

PVince81 mentioned this pull request May 26, 2017

Email templates cannot be loaded. #27628

Closed

PVince81 modified the milestones: 10.0.3, 10.0.2 May 26, 2017

DeepDiver1975 mentioned this pull request Jul 13, 2017

theming l10n translation #28167

Closed

felixboehm added p1-urgent Critical issue, need to consider hotfix with just that issue feature:theming labels Jul 17, 2017

Philipp Schaffrath and others added 5 commits August 10, 2017 09:05

added functionality to get an array of all themes and search for a th…

0ac4c97

…eme, refactored theme object so it can also set the web path through constructor, added setters to manipulate the current theme, removed unnecessary slash, added/modified tests

created public interfaces for Theme and ThemeService, fixed some docb…

aee0ea3

…locks, implemented interfaces

use ThemeService and Theme interfaces internally

5380c9f

added alias for IThemeService

5e31acd

use fully qualified interface name

1f25c61

PVince81 force-pushed the extend-theme-service branch from c14cdc0 to 1f25c61 Compare August 10, 2017 07:06

PVince81 merged commit 7c587c8 into master Aug 10, 2017

PVince81 deleted the extend-theme-service branch August 10, 2017 19:01

PVince81 mentioned this pull request Aug 10, 2017

[stable10] extended theme service #28647

Merged

lock Bot locked as resolved and limited conversation to collaborators Aug 2, 2019

Uh oh!

Conversation

phisch commented Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

PVince81 commented May 4, 2017

Uh oh!

phisch commented May 5, 2017

Uh oh!

PVince81 May 5, 2017

Choose a reason for hiding this comment

Uh oh!

phisch Jul 12, 2017

Choose a reason for hiding this comment

Uh oh!

PVince81 commented May 5, 2017

Uh oh!

phisch commented May 5, 2017

Uh oh!

DeepDiver1975 left a comment

Choose a reason for hiding this comment

Uh oh!

PVince81 commented May 15, 2017

Uh oh!

phisch commented May 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DeepDiver1975 commented May 16, 2017

Uh oh!

DeepDiver1975 commented May 17, 2017

Uh oh!

PVince81 commented May 17, 2017

Uh oh!

PVince81 commented May 17, 2017

Uh oh!

DeepDiver1975 May 18, 2017

Choose a reason for hiding this comment

Uh oh!

PVince81 commented May 19, 2017

Uh oh!

phisch commented May 19, 2017

Uh oh!

PVince81 commented May 19, 2017

Uh oh!

PVince81 commented May 19, 2017

Uh oh!

PVince81 commented Jul 4, 2017

Uh oh!

phisch commented Jul 12, 2017

Uh oh!

DeepDiver1975 commented Jul 12, 2017

Uh oh!

phisch commented Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Aug 3, 2017

Uh oh!

PVince81 commented Aug 10, 2017

Uh oh!

PVince81 commented Aug 10, 2017

Uh oh!

lock Bot commented Aug 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

phisch commented Apr 26, 2017 •

edited

Loading

phisch commented May 16, 2017 •

edited

Loading

phisch commented Jul 13, 2017 •

edited

Loading