Allow non hardcoded icons for apps by sharidas · Pull Request #29013 · owncloud/core

sharidas · 2017-09-14T16:27:37Z

This change helps users to provide their own
custom icons for the apps which work with
ownCloud.

Signed-off-by: Sujith H sharidasan@owncloud.com

Description

This change helps contributors to use non-hardcoded icons. That is every app developer or contributor can now have their own separate icons which can be used with core.

Related Issue

#28751

Motivation and Context

At present the icons are hard coded and are located at core. This prohibits the usage of custom icons for apps. With this change, contributors of apps, can have their own custom app which can be used along with core. No more saving of svg files in the core.

How Has This Been Tested?

I have verified it against 2 apps. One is encryption app ( the change was made in the SettingsManager.php) and another app was firewall app. For the firewall app I modified the AdminSection.php to test.

Screenshots (if appropriate):

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.

tomneedham · 2017-09-15T10:26:05Z

lib/public/Settings/ISection.php

@@ -53,4 +53,6 @@ public function getPriority();
 	 */
 	public function getIconName();


I think let's kill the name and just keep the url and we'll make it backwards compatible

tomneedham · 2017-09-15T10:26:42Z

settings/Controller/SettingsPageController.php

 				'name' => ucfirst($section->getName()),
 				'active' => $section->getID() === $currentSectionID,
-				'icon' => $section->getIconName()
+				'icon' => $section->getIconName(),


so we can get rid of this also

tomneedham · 2017-09-15T10:27:40Z

lib/private/legacy/app.php

+		$url = strstr($fileAbsolutePath, \OC::$WEBROOT);
+		return $url;
+	}
+


nooooo more static ;) Why do we need to do this copying stuff?

The intention behind this change was to fetch the data from the web url and download to apps/img folder. Anything wrong?

tomneedham · 2017-09-15T10:30:45Z

lib/private/Settings/Section.php


+	public function getIconUrl() {
+		return \OC_App::getIconFromUrl($this->url, $this->getID());
+	}


I think we could use the urlgenerator here, injected in properly

sharidas · 2017-09-15T11:54:47Z

Attaching new screenshot for the change made:

tomneedham · 2017-09-15T12:16:28Z

lib/private/Settings/Section.php

we can pass in the urlgenerator here - then adjust the constructor to pass it in in \OC\Settings\SettingsManager

tomneedham · 2017-09-15T12:16:57Z

lib/private/Settings/Section.php

use passed in urlgenerator. Also, the Id is not necessarily the app id so might need some thought here

tomneedham · 2017-09-15T12:18:56Z

settings/templates/settingsPage.php

we may as well standardise the core icons. So always pass a url. In the controller figure out if its a core icon or not. Then we can remove these crappy css rules for the icons

tomneedham · 2017-09-15T14:01:02Z

lib/private/Settings/SettingsManager.php

missing this one?

tomneedham · 2017-09-15T14:01:27Z

lib/public/Settings/ISection.php

and we should keep the old method, but add @deprecated from 10.0.4. But this shoudl also be backwards compatible. So inside the section implementation, set $this->url using the url generator - don't need to worry about injection for this backwards compat

So if we add @deprecated for getIconName in ISection then, I think we will end up having both getIconName and getIconUrl. Because the apps which will be implementing ISection would need to implement both the methods.
So ideally if we don't want to deprecate it, then may be we can have getIconName. And make it app developers responsibility to get the url to be generated and returned from getIconName(), instead of just returning the icon name which it does currently.

sharidas · 2017-09-15T14:24:52Z

lib/public/Settings/ISection.php

 	/**
-	 * @since 10.0
-	 * @return string
+	 * @deprecated since 10.0.4


Added @deprecated

tomneedham · 2017-09-15T15:27:25Z

we should update the docs here https://doc.owncloud.org/server/latest/developer_manual/core/theming.html?highlight=theme#create-custom-sections

DeepDiver1975 · 2017-09-27T07:07:27Z

lib/public/Settings/ISection.php

 	 * @return string
 	 */
-	public function getIconName();
+	public function getIconUrl();


I don't like this - this will break all existing implementations 👎

We should implement this more in sense where the returned icon name can either be a css class (as done until today) or an image name of the app which hosts the svg.

this way we don't need to change apis and implementation all over the place.

$icon = $section->getIconName(); if (file_exists("$appPath/img/$icon.svg")) { $url = $urlGenerator->imagePath("$icon.svg"); // TODO: add background styling hack } else { // TODO: add icon css class }

jvillafanez · 2017-10-05T11:53:10Z

settings/Controller/SettingsPageController.php

+			$appPath = \OC_App::getAppPath($section->getID());
+
+			if (file_exists($appPath . '/img/' . $icon)) {
+				$icon = \OC::$server->getURLGenerator()->imagePath($section->getID(), $icon);


The URLGenerator should be injected in the constructor

jvillafanez · 2017-10-05T12:08:38Z

settings/Controller/SettingsPageController.php

 		// Iterate through sections and get id, name and see if currently active
 		foreach($sections as $section) {
+
+			$icon = $section->getIconName() . '.svg';


Move this "getIcon" code to another function and call it here. This logic should be probably out of this class so we can unittest it properly. If there is no place for this, it should be fine to create a private function here in the short term.

In addition, I think there is already a function to retrieve icons for apps (I might be wrong) Any reason to not use it?

In the phpdoc of the new function (whenever is placed) include how the function should behave. Right now I'm not sure what is the expected behaviour if several icons are provided: if I want to use "marvellous_icon.png", why I'm checking "marvellous_icon.png.svg"?
If only .svg files are supported, make sure this is known, including in the comments of the function.
Since I have no idea what is the expected behaviour here, I'm more inclined to think the current behaviour is buggy.

A couple of extra things:

Unless the icon name is already sanitized, make sure it doesn't contain /../ or something similar

I'm not sure what is the "default" expectation: I guess it should be using the filename without extension as icon name (which is fine). In any case, optimize for this default case. If the default is to include the extension you'd be checking for a missing file constantly. Just something to take care of.

jvillafanez · 2017-10-05T12:57:03Z

settings/templates/settingsPage.php

 		<?php foreach($_['personalNav'] as $item) {
 			$active = $item['active'] ? ' active ' : '';
+
+			$ocUrlorIcon = \OCP\Util::sanitizeHTML($item['icon']);


This could be a good time to rewrite this piece to just use this as a template and avoid adding logic here.

Agreed. Can we do this as a separate PR?

I don't really mind, but there will be conflicts with this PR if t isn't merged soon because that code will depend on this branch... @tomneedham @DeepDiver1975 opinions?

do it in here - own commit for now to see the diff ... later we squash

Moved the logic from here to https://github.com/owncloud/core/pull/29013/files#diff-3393e43384d070ab12865348f9279030R123. I hope this makes sense now.

jvillafanez · 2017-10-06T12:43:17Z

The rewrite I had in mind was more like:

<li class="divider"><?php p($l->t('Personal')); ?></li>
<?php foreach($_['personalNav'] as $item): ?>
<li>
  <?php if ($item['active']): ?>
    <?php if ($item['iconIsUrl]): ?>
      <a class="svg active" style="background-image: url(<?php p($item['icon']) ?>)" href='<?php p($item['link']); ?>'><?php p($item['name']) ?></a>
    <?php else: ?>
      <a class="svg active icon-<?php p($item['icon']) ?>)" href='<?php p($item['link']); ?>'><?php p($item['name']) ?></a>
    <?php endif; ?>
  <?php else: ?>
    <?php if ($item['iconIsUrl]): ?>
      <a class="svg" style="background-image: url(<?php p($item['icon']) ?>)" href='<?php p($item['link']); ?>'><?php p($item['name']) ?></a>
    <?php else: ?>
      <a class="svg icon-<?php p($item['icon']) ?>)" href='<?php p($item['link']); ?>'><?php p($item['name']) ?></a>
  <?php endif; ?>
</li>
<?php endforeach; ?>

Maybe we can make it smaller injecting "active" or an empty string instead of checking if the $item['active'] is a boolean or not, but as you see, the template just output information with the minimum control structures.

sharidas · 2017-10-06T15:04:31Z

@jvillafanez I have updated the change let me know if this ok.

jvillafanez · 2017-10-09T06:47:09Z

settings/templates/settingsPage.php

\OCP\Util::sanitizeHTML(...) isn't needed with the p function

Removed the call to sanitizeHTML

jvillafanez · 2017-10-09T06:49:23Z

settings/templates/settingsPage.php

indentation here seems misplaced

sharidas · 2017-10-09T06:56:57Z

@jvillafanez Corrected indentation and removed call to \OCP\Util::sanitizeHTML() inside p()

jvillafanez

Code looks good to me, not tested.

sharidas · 2017-10-09T07:25:28Z

Tested the code by replacing icon password with docker. I copied docker.svg from https://worldvectorlogo.com/download/docker.svg and the settings page looks like this:

And when the change is reverted back to password at https://github.com/owncloud/core/blob/master/lib/private/Settings/SettingsManager.php#L204 the settings page encryption icon looks like this:

codecov · 2017-10-20T00:26:16Z

Codecov Report

Merging #29013 into master will increase coverage by 0.01%.
The diff coverage is 61.53%.

@@             Coverage Diff              @@
##             master   #29013      +/-   ##
============================================
+ Coverage     60.21%   60.23%   +0.01%     
- Complexity    17182    17184       +2     
============================================
  Files          1030     1030              
  Lines         57271    57263       -8     
============================================
+ Hits          34485    34491       +6     
+ Misses        22786    22772      -14

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Settings/SettingsManager.php	`72.72% <ø> (-0.18%)`	`44 <0> (ø)`
settings/templates/settingsPage.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
settings/Controller/SettingsPageController.php	`70.68% <88.88%> (+2.68%)`	`14 <2> (+2)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8d5f70...cdd2692. Read the comment docs.

This change helps users to provide their own custom icons for the apps which work with ownCloud. Signed-off-by: Sujith H <sharidasan@owncloud.com>

PVince81 · 2017-10-26T09:12:20Z

lib/private/Settings/SettingsManager.php

 				new Section('workflow', $this->l->t('Workflows & Tags'), 85, 'workflows'),
 				new Section('sharing', $this->l->t('Sharing'), 80, 'share'),
 				new Section('search', $this->l->t('Search'), 75, 'search'),
-				new Section('updates', $this->l->t('Updates'), 20, 'update'),


why remove this ?

While searching for update.svg, I couldn't find at core. That made me remove this line. I can revert back the change, if my finding was wrong.

Thanks for the explanation, I just wanted to double check that this wasn't a mistake. Let's keep it.

PVince81 · 2017-10-26T09:20:19Z

@sharidas please backport

sharidas · 2017-10-26T09:45:33Z

Created backport PR: #29358

lock · 2019-08-02T05:01:47Z

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.

sharidas changed the title ~~Allow non hardcoded icons for apps~~ [WIP] Allow non hardcoded icons for apps Sep 14, 2017

sharidas self-assigned this Sep 14, 2017

sharidas added the 2 - Developing label Sep 14, 2017

sharidas added this to the development milestone Sep 14, 2017

sharidas requested review from PVince81 and tomneedham September 14, 2017 16:35

tomneedham suggested changes Sep 15, 2017

View reviewed changes

sharidas force-pushed the non-hard-coded-icons branch from d8b4dde to 5050bcb Compare September 15, 2017 11:50

sharidas force-pushed the non-hard-coded-icons branch from 5050bcb to d7306ae Compare September 15, 2017 12:08

tomneedham suggested changes Sep 15, 2017

View reviewed changes

sharidas force-pushed the non-hard-coded-icons branch from d7306ae to 570cb64 Compare September 15, 2017 13:49

tomneedham reviewed Sep 15, 2017

View reviewed changes

lib/private/Settings/SettingsManager.php Outdated

Copy link

Contributor

tomneedham Sep 15, 2017

Choose a reason for hiding this comment

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

missing this one?

tomneedham reviewed Sep 15, 2017

View reviewed changes

sharidas force-pushed the non-hard-coded-icons branch from 570cb64 to 8835320 Compare September 15, 2017 14:23

sharidas commented Sep 15, 2017

View reviewed changes

PVince81 modified the milestones: 10.0.3, development Sep 18, 2017

sharidas force-pushed the non-hard-coded-icons branch 2 times, most recently from fff6f2a to 2b5898f Compare September 19, 2017 12:56

DeepDiver1975 requested changes Sep 27, 2017

View reviewed changes

sharidas force-pushed the non-hard-coded-icons branch from 2b5898f to b66a621 Compare September 27, 2017 10:10

sharidas changed the title ~~[WIP] Allow non hardcoded icons for apps~~ Allow non hardcoded icons for apps Sep 27, 2017

sharidas added 3 - To Review and removed 2 - Developing labels Sep 27, 2017

sharidas force-pushed the non-hard-coded-icons branch from b66a621 to 5225596 Compare September 27, 2017 10:19

jvillafanez reviewed Oct 5, 2017

View reviewed changes

sharidas force-pushed the non-hard-coded-icons branch 2 times, most recently from b2d04f0 to 204fff3 Compare October 6, 2017 11:52

sharidas force-pushed the non-hard-coded-icons branch from 204fff3 to b85a585 Compare October 6, 2017 15:02

sharidas force-pushed the non-hard-coded-icons branch from b85a585 to d40e54d Compare October 9, 2017 06:26

jvillafanez reviewed Oct 9, 2017

View reviewed changes

sharidas force-pushed the non-hard-coded-icons branch from d40e54d to 28f74bd Compare October 9, 2017 06:55

jvillafanez approved these changes Oct 9, 2017

View reviewed changes

DeepDiver1975 force-pushed the non-hard-coded-icons branch from 28f74bd to 0e0d71e Compare October 19, 2017 18:26

Allow non hardcoded icons for apps

cdd2692

This change helps users to provide their own custom icons for the apps which work with ownCloud. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas force-pushed the non-hard-coded-icons branch from 0e0d71e to cdd2692 Compare October 26, 2017 02:59

PVince81 reviewed Oct 26, 2017

View reviewed changes

PVince81 merged commit 2a6594d into master Oct 26, 2017

PVince81 deleted the non-hard-coded-icons branch October 26, 2017 09:20

PVince81 added the backport-request label Oct 26, 2017

sharidas mentioned this pull request Oct 26, 2017

[stable10] Allow non hardcoded icons for apps #29358

Merged

9 tasks

PVince81 removed the backport-request label Nov 3, 2017

ownclouders added the status/STALE label Dec 20, 2017

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

		@@ -53,4 +53,6 @@ public function getPriority();
		*/
		public function getIconName();

Conversation

sharidas commented Sep 14, 2017

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharidas commented Sep 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomneedham Sep 15, 2017 • edited by phil-davis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomneedham commented Sep 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvillafanez commented Oct 6, 2017

Uh oh!

sharidas commented Oct 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharidas commented Oct 9, 2017

Uh oh!

jvillafanez left a comment

Choose a reason for hiding this comment

Uh oh!

sharidas commented Oct 9, 2017

tomneedham Sep 15, 2017 •

edited by phil-davis

Loading

codecov bot commented Oct 20, 2017 •

edited

Loading