Theming: Add preview and hide undo buttons #770

juliusknorr · 2016-08-08T16:36:54Z

This pull requests adds the following to the theming app:

show a preview of the log in image for feedback that it’s different Theming app: preview for log in background image #217
for the input fields, only show the reset button when the respective field is focused.

mention-bot · 2016-08-08T16:36:55Z

@juliushaertl, thanks for your PR! By analyzing the annotation information on this pull request, we identified @schiessle, @LukasReschke and @nickvergessen to be potential reviewers

juliusknorr · 2016-08-08T18:55:01Z

OK, CI seems to be happy, please review.

cc @jancborchardt @nextcloud/theming

LukasReschke · 2016-08-09T22:22:19Z

apps/theming/lib/Template.php

+	public function getLogo() {
+		$logo = $this->config->getAppValue('theming', 'logoMime');
+		$pathToLogo = $this->config->getSystemValue('datadirectory', \OC::$SERVERROOT . '/data/') . '/themedinstancelogo';
+		if(!$logo || !file_exists($pathToLogo)) {


Can you use IRootFolder for that? @schiessle told me about some object store magic and so on that otherwise would not be compatible.

Yes, always use the Nextcloud file system abstraction to make sure that it works with all underlying storages.

LukasReschke · 2016-08-09T22:22:54Z

I like it 👍

@schiessle Mind taking a look at my remarks? Same as we had in the ThemingController itself

MorrisJobke · 2016-08-09T23:52:22Z

I tested this and it works 👍 (only for translated slogans this doesn't work, because somehow the lib translation files aren't loaded -> but this is not caused by the code in here)

schiessle · 2016-08-10T08:21:35Z

As soon as the comments by @LukasReschke are addressed this can be merged.

juliusknorr · 2016-08-10T11:36:11Z

I've just pushed another commit. Please check if it is ok like this @LukasReschke @schiessle 😃

juliusknorr · 2016-08-10T13:02:34Z

Hmm tests are failing, but I don't have any idea why:

There were 3 errors:

1) Test\Files\ViewTest::testLockBasicOperation with data set #4 ('rmdir', array('dir'), 'dir', 'delete', 1, 2, 1)
OCP\Lock\LockedException: "dir" is locked

/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:1956
/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:1142
/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:342
/drone/src/github.com/nextcloud/server/tests/lib/Files/ViewTest.php:1793

Caused by
OCP\Lock\LockedException: "files/a2e47ee39f947ec3961ccd7c88e6472b" is locked

/drone/src/github.com/nextcloud/server/lib/private/Lock/MemcacheLockingProvider.php:117
/drone/src/github.com/nextcloud/server/lib/private/Files/Storage/Common.php:669
/drone/src/github.com/nextcloud/server/lib/private/Files/Storage/Wrapper/Wrapper.php:606
/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:1952
/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:1142
/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:342
/drone/src/github.com/nextcloud/server/tests/lib/Files/ViewTest.php:1793

2) Test\Files\ViewTest::testLockBasicOperation with data set #5 ('unlink', array('test.txt'), 'test.txt', 'delete', 1, 2, 1)
OCP\Lock\LockedException: "test.txt" is locked

/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:1956
/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:1142
/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:691
/drone/src/github.com/nextcloud/server/tests/lib/Files/ViewTest.php:1793

Caused by
OCP\Lock\LockedException: "files/c56e89314d7862aaf193d5ef7e8fa32a" is locked

/drone/src/github.com/nextcloud/server/lib/private/Lock/MemcacheLockingProvider.php:117
/drone/src/github.com/nextcloud/server/lib/private/Files/Storage/Common.php:669
/drone/src/github.com/nextcloud/server/lib/private/Files/Storage/Wrapper/Wrapper.php:606
/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:1952
/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:1142
/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:691
/drone/src/github.com/nextcloud/server/tests/lib/Files/ViewTest.php:1793

3) OCA\Files\Tests\Command\DeleteOrphanedFilesTest::testClearFiles
OCP\Files\NotFoundException: test not found while trying to get owner

/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:1644
/drone/src/github.com/nextcloud/server/lib/private/Files/Filesystem.php:890
/drone/src/github.com/nextcloud/server/apps/files_trashbin/lib/Trashbin.php:83
/drone/src/github.com/nextcloud/server/apps/files_trashbin/lib/Trashbin.php:205
/drone/src/github.com/nextcloud/server/apps/files_trashbin/lib/Storage.php:161
/drone/src/github.com/nextcloud/server/apps/files_trashbin/lib/Storage.php:96
/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:1120
/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:691
/drone/src/github.com/nextcloud/server/apps/files/tests/Command/DeleteOrphanedFilesTest.php:126

LukasReschke · 2016-08-10T15:25:16Z

cc @icewind1991 any idea? 🚀

jancborchardt · 2016-08-11T11:03:10Z

apps/theming/css/settings-admin.css

@@ -32,3 +32,17 @@
 div#theming_settings_msg {
    margin-left: 10px;
 }
+
+#theming-preview {
+    width:230px;


Just a nitpick, but coding style here and below: Always a space after the : colon. ;)

schiessle · 2016-08-15T08:07:46Z

any news here... @icewind1991 did you had a chance to look at the failing tests?

juliusknorr · 2016-08-19T13:08:08Z

Ok, i've just rebased this to fix the conflicts with the new admin panel. Tests are still failing, but when I run "drone exec" locally everything works fine. Could there be an issue with the CI server?

cc @icewind1991 @MorrisJobke

MorrisJobke · 2016-08-19T14:28:03Z

It's in $ NOCOVERAGE=true TEST_SELECTION=DB ./autotest.sh sqlite

There were 3 errors:

1) Test\Files\ViewTest::testLockBasicOperation with data set #4 ('rmdir', array('dir'), 'dir', 'delete', 1, 2, 1)
OCP\Lock\LockedException: "dir" is locked

/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:1956
/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:1142
/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:342
/drone/src/github.com/nextcloud/server/tests/lib/Files/ViewTest.php:1793

Caused by
OCP\Lock\LockedException: "files/a2e47ee39f947ec3961ccd7c88e6472b" is locked

/drone/src/github.com/nextcloud/server/lib/private/Lock/MemcacheLockingProvider.php:117
/drone/src/github.com/nextcloud/server/lib/private/Files/Storage/Common.php:669
/drone/src/github.com/nextcloud/server/lib/private/Files/Storage/Wrapper/Wrapper.php:606
/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:1952
/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:1142
/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:342
/drone/src/github.com/nextcloud/server/tests/lib/Files/ViewTest.php:1793

2) Test\Files\ViewTest::testLockBasicOperation with data set #5 ('unlink', array('test.txt'), 'test.txt', 'delete', 1, 2, 1)
OCP\Lock\LockedException: "test.txt" is locked

/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:1956
/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:1142
/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:691
/drone/src/github.com/nextcloud/server/tests/lib/Files/ViewTest.php:1793

Caused by
OCP\Lock\LockedException: "files/c56e89314d7862aaf193d5ef7e8fa32a" is locked

/drone/src/github.com/nextcloud/server/lib/private/Lock/MemcacheLockingProvider.php:117
/drone/src/github.com/nextcloud/server/lib/private/Files/Storage/Common.php:669
/drone/src/github.com/nextcloud/server/lib/private/Files/Storage/Wrapper/Wrapper.php:606
/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:1952
/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:1142
/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:691
/drone/src/github.com/nextcloud/server/tests/lib/Files/ViewTest.php:1793

3) OCA\Files\Tests\Command\DeleteOrphanedFilesTest::testClearFiles
OCP\Files\NotFoundException: test not found while trying to get owner

/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:1644
/drone/src/github.com/nextcloud/server/lib/private/Files/Filesystem.php:890
/drone/src/github.com/nextcloud/server/apps/files_trashbin/lib/Trashbin.php:83
/drone/src/github.com/nextcloud/server/apps/files_trashbin/lib/Trashbin.php:205
/drone/src/github.com/nextcloud/server/apps/files_trashbin/lib/Storage.php:161
/drone/src/github.com/nextcloud/server/apps/files_trashbin/lib/Storage.php:96
/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:1120
/drone/src/github.com/nextcloud/server/lib/private/Files/View.php:691
/drone/src/github.com/nextcloud/server/apps/files/tests/Command/DeleteOrphanedFilesTest.php:126

Tests are still failing, but when I run "drone exec" locally everything works fine. Could there be an issue with the CI server?

usually not, because it also doesn't do anything different 😕 Let me try it locally too.

MorrisJobke · 2016-08-19T14:42:57Z

Works here too ... I restarted the job at the drone instance ... maybe it was a bad race condition due to load.

nickvergessen · 2016-08-22T07:48:19Z

I guess it's related to the loading order

nickvergessen · 2016-08-23T09:11:33Z

apps/theming/css/settings-admin.css

+    max-width: 20%;
+    max-height: 20%;
+    margin-top: 20px;
+}


new line missing

juliusknorr · 2016-08-23T19:41:20Z

OK, I'll close this PR and try to split it up and resubmit.
Maybe that will help, I've no idea why these tests are just failing for this PR.

juliusknorr added enhancement 2. developing Work in progress feature: theming labels Aug 8, 2016

juliusknorr added this to the Nextcloud 11.0 milestone Aug 8, 2016

juliusknorr mentioned this pull request Aug 8, 2016

Theming: Fix missing color usage #771

Merged

5 tasks

juliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 8, 2016

LukasReschke reviewed Aug 9, 2016
View reviewed changes

MorrisJobke mentioned this pull request Aug 9, 2016

Release review polishing #735

Closed

67 tasks

juliusknorr force-pushed the theming-improvements branch from 5447965 to 232cd38 Compare August 10, 2016 12:13

jancborchardt mentioned this pull request Aug 11, 2016

All theming issues still to be fixed #454

Closed

10 tasks

jancborchardt reviewed Aug 11, 2016
View reviewed changes

juliusknorr force-pushed the theming-improvements branch from 232cd38 to 5eb5d1b Compare August 11, 2016 11:41

rullzer force-pushed the theming-improvements branch from 5eb5d1b to 8e5eb01 Compare August 13, 2016 12:20

juliusknorr mentioned this pull request Aug 14, 2016

Add dynamic icon creation #840

Merged

7 tasks

LukasReschke assigned icewind1991 Aug 15, 2016

juliusknorr force-pushed the theming-improvements branch from 8e5eb01 to cc9dac3 Compare August 16, 2016 13:17

juliusknorr added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Aug 19, 2016

juliusknorr force-pushed the theming-improvements branch from cc9dac3 to 0e1b5d4 Compare August 19, 2016 11:49

juliusknorr closed this Aug 22, 2016

juliusknorr force-pushed the theming-improvements branch from 0e1b5d4 to 91e8b33 Compare August 22, 2016 19:45

juliusknorr reopened this Aug 22, 2016

nickvergessen reviewed Aug 23, 2016
View reviewed changes

apps/theming/css/settings-admin.css

max-width: 20%;

max-height: 20%;

margin-top: 20px;

}

Copy link

Member

nickvergessen Aug 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new line missing

juliusknorr added 5 commits August 23, 2016 21:29

Theming: Add preview for login screen

809b4f2

Theming: Hide undo button on default values

119dbf6

Add unit tests ans small js fixes

c103b4e

Use IRootFolder for checking image existence

338c7df

Fix CSS code style

086b6ab

juliusknorr force-pushed the theming-improvements branch from ba6698d to 086b6ab Compare August 23, 2016 19:30

juliusknorr closed this Aug 23, 2016

juliusknorr deleted the theming-improvements branch August 23, 2016 19:41

This was referenced Aug 23, 2016

Theming: Add logo and background to ThemingDefaults #1026

Merged

Theming admin improvements #1049

Merged

MorrisJobke mentioned this pull request Jan 23, 2017

Show reset to default button for theming values on hover #3198

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Theming: Add preview and hide undo buttons #770

Theming: Add preview and hide undo buttons #770

juliusknorr commented Aug 8, 2016 •

edited

Loading

mention-bot commented Aug 8, 2016

juliusknorr commented Aug 8, 2016 •

edited

Loading

LukasReschke Aug 9, 2016

schiessle Aug 10, 2016

LukasReschke commented Aug 9, 2016

MorrisJobke commented Aug 9, 2016

schiessle commented Aug 10, 2016

juliusknorr commented Aug 10, 2016

juliusknorr commented Aug 10, 2016

LukasReschke commented Aug 10, 2016

jancborchardt Aug 11, 2016

schiessle commented Aug 15, 2016

juliusknorr commented Aug 19, 2016

MorrisJobke commented Aug 19, 2016

MorrisJobke commented Aug 19, 2016

nickvergessen commented Aug 22, 2016

nickvergessen Aug 23, 2016

juliusknorr commented Aug 23, 2016

Theming: Add preview and hide undo buttons #770

Theming: Add preview and hide undo buttons #770

Conversation

juliusknorr commented Aug 8, 2016 • edited Loading

mention-bot commented Aug 8, 2016

juliusknorr commented Aug 8, 2016 • edited Loading

LukasReschke Aug 9, 2016

Choose a reason for hiding this comment

schiessle Aug 10, 2016

Choose a reason for hiding this comment

LukasReschke commented Aug 9, 2016

MorrisJobke commented Aug 9, 2016

schiessle commented Aug 10, 2016

juliusknorr commented Aug 10, 2016

juliusknorr commented Aug 10, 2016

LukasReschke commented Aug 10, 2016

jancborchardt Aug 11, 2016

Choose a reason for hiding this comment

schiessle commented Aug 15, 2016

juliusknorr commented Aug 19, 2016

MorrisJobke commented Aug 19, 2016

MorrisJobke commented Aug 19, 2016

nickvergessen commented Aug 22, 2016

nickvergessen Aug 23, 2016

Choose a reason for hiding this comment

juliusknorr commented Aug 23, 2016

juliusknorr commented Aug 8, 2016 •

edited

Loading

juliusknorr commented Aug 8, 2016 •

edited

Loading