Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop disabling the theme on app updates! #19271

Closed
oparoz opened this issue Sep 22, 2015 · 13 comments
Closed

Stop disabling the theme on app updates! #19271

oparoz opened this issue Sep 22, 2015 · 13 comments

Comments

@oparoz
Copy link
Contributor

oparoz commented Sep 22, 2015

Every time you update an app, you lose the theme. That key needs to be protected when updating apps.

Steps to reproduce

  1. Install your theme
  2. Update app
  3. Look at your new login page

Actual result

Theme is gone ;(

Expected result

Login page looks just like you've designed it.

@PVince81
Copy link
Contributor

Makes sense

@DeepDiver1975
Copy link
Member

Well - we disable the theme because it can break the installation.

Which can also happen with an app upgrade because the theming capabilities allow to overwrite templates of an app.

This is a tricky beast.

@DeepDiver1975
Copy link
Member

in addition this is related to upgrade which will be a major topic for 9.0 -> moving ...

@DeepDiver1975 DeepDiver1975 modified the milestones: 9.0-next, 8.2-current Sep 22, 2015
@oparoz
Copy link
Contributor Author

oparoz commented Sep 22, 2015

OK. I think the least the updater could do is to offer to turn the theme back on at the end. If it breaks then, the admin can remove it from the config to fix things.

@deMattin
Copy link

The problem might be, that your theme breaks or simply only changes something, you don't see at first view.
My theme also fixes some issues (or things, that only I see as an issue like linespacing in files app) and it is always a good idea, to go through all apps thoroughly with original theme first, to see, how it looks like now and THEN activate your personal theme.
Only this way you recognise, if your theming does only those things, you want to be done.

It's the more important to do it like this, the more and complex the theming is, you use.
If you only change some grafics or the simple login screen, it maybe(?) doesn't matter. But it's different, if you also (have to) theme "active" files like js files.

If upgrade won't reset theme, it would be harder to live this good practise some users even will do without knowing a reason for this - only because they check oc after upgrade and later after that activate their theme.

Best way to solve this would be, to only reset theming, if the update/upgrade changed something in the files you themed.
But for this it would be some coding neccessary and core work to be done. I think actually everything is deleted and newly uploaded during an upgrade independend of the fact, a file has changed or not.

So in my point of view, an upgrade should reset the theme, if only because it is to remind you, that you use one and have to check everything VERY well after update/upgrade.

-> It's not a bug, it's a feature 😉

@oparoz
Copy link
Contributor Author

oparoz commented Oct 29, 2015

Best way to solve this would be, to only reset theming, if the update/upgrade changed something in the files you themed.

I think this would be too hard to implement, but I don't know if we can theme specific apps directly from core if we can then the theme should definitely not be disabled if an un-themed app is updated.

it is always a good idea, to go through all apps thoroughly with original theme first, to see, how it looks like now and THEN activate your personal theme.

Yes, but you can do this on the dev instance and then you should be able to update the production one without having to edit the config afterwards.

@ghost ghost modified the milestones: 9.1-next, 9.0-current Feb 22, 2016
@jancborchardt
Copy link
Member

So what’s the call on this @PVince81 @DeepDiver1975?

@PVince81 PVince81 modified the milestones: 9.1-current, 9.1.1-next-maintenance Jun 30, 2016
@PVince81 PVince81 modified the milestones: 9.1.2, 9.1.1 Sep 21, 2016
@PVince81 PVince81 modified the milestones: 9.1.3, 9.1.2 Oct 20, 2016
@PVince81 PVince81 modified the milestones: 9.2, 9.1.3 Nov 30, 2016
@PVince81
Copy link
Contributor

PVince81 commented Apr 6, 2017

@phisch likely obsolete.

What happens on update when a theme app is enabled ? I guess it stays enabled and is also able to style the update page ?

@PVince81 PVince81 closed this as completed Apr 6, 2017
@phisch
Copy link
Contributor

phisch commented Apr 7, 2017

@PVince81 The theme app is not loaded during the update screen. Also it is disabled after an update, and must be enabled manually again. Should we improve this?

@PVince81
Copy link
Contributor

PVince81 commented Apr 7, 2017

Is it disabled as part of the "disable third party apps" logic I guess ?

In this case I'd leave this as part of #14754 (comment) or other topics related to apps and updates.

@phisch
Copy link
Contributor

phisch commented Apr 7, 2017

@PVince81 AFAIK the theme should get disabled after an update, since themes can overwrite templates, and an update could provide new functionality which would be missing if the theme was still enabled.

I personally think that the theme should stay active. The owncloud admin should know what he has overwritten in a theme. Also overwriting templates should be rare for a theme.

@PVince81
Copy link
Contributor

PVince81 commented Apr 7, 2017

I'd expect new functionaly only on major versions, not minor.

That's why in that linked ticket I suggested to only disable apps on major updates.

@lock
Copy link

lock bot commented Aug 2, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants