XWIKI-14342: Make Extension Manager use IconThemes #2823

Sereza7 · 2024-01-25T16:10:16Z

Jira URL

https://jira.xwiki.org/browse/XWIKI-14342

Changes

Description

Removed the Hardcoded silk icons from the CSS
Fixed style to fit actual icon nodes in the same place as they used to be as background.
Added them in the velocity template instead
Fixed the behavior of status switch when loading dependencies.

Clarifications

Adding a spinner dynamically was already difficult enough, so I decided to just use the default xwiki file instead of going for an approach that would react to the current iconTheme, like the one in the velocity template.

Screenshots & Video

Here is a demo of the changes brought in this PR. In the first part, we can see that the looks with the silk iconTheme are the same as they used to be. However in the second part, we can see that now Font Awesome icons are correctly used when FontAwesome is set as the iconTheme.

Executed Tests

No test executed exept manual tests similar to the one in the demo.

Expected merging strategy

Prefers squash: Yes
Backport on branches:
- 15.10.X

* Removed the Hardcoded silk icons from the CSS * Fixed style to fit actual icon nodes in the same place as they used to be as background. * Added them in the velocity template instead * Fixed the behavior of status switch when loading dependencies.

...orm-web/xwiki-platform-web-war/src/main/webapp/resources/uicomponents/extension/extension.js

manuelleduc · 2024-12-17T08:32:44Z

...orm-web/xwiki-platform-web-war/src/main/webapp/resources/uicomponents/extension/extension.js

@@ -495,6 +495,12 @@ XWiki.ExtensionBehaviour = Class.create({
      parameters : formData,
      onCreate : function() {
        dependency.removeClassName('extension-item-unknown').addClassName('extension-item-loading');
+        // Remove the unknown icon.


This is not really clear why we need to do all this.
Also, I don't know this code much but why do you need to manipulate Velocity code from javascript?

Also, I don't know this code much but why do you need to manipulate Velocity code from javascript?

We could make it work with smart CSS relying on logic classes to hide and show icons. I tried to keep the changes to a minimum to avoid making this more complex. Keeping things simple means also that it's less likely to break things in a usecase/customization unknown to me.

This code is quite old and large. A complete overhaul would be the best solution quality wise, but it's beyond the goal of this PR.

This is not really clear why we need to do all this.

Before, the icon would swap directly when we swapped the classes. Here, I added a few lines to swap the icon DOM elements at the same time as they did before.

TODO: At least add a comment to explain the HTML manipulations. Should probably add CSS logic to avoid this HTML manipulations. Do leave things like this if extensive testing show that it's not possible to have a simple CSS logic in place...

manuelleduc · 2024-12-17T08:36:23Z

...re/xwiki-platform-web/xwiki-platform-web-templates/src/main/resources/templates/extension.vm

@@ -1038,6 +1069,24 @@ $namespace##
    </div>
    <div class="extension-header">
      <h2 class="extension-title">
+        #if($!extensionStatus == 'core' ||


I don't like too much this duplicated logic. @tmortagne do you know if we have velocity services for this?

AFAIK, this concept of status is something @mflorea introduced in #determineExtensionStatus, and it does not exist on Java side.

Note that the logic is not duplicated, it looks very similar, but e.g. there's also the incompatible ones for dependencies.
This solution is very similar to what was done in CSS before (see the css removed from extension.css, it wasn't really refined either ^^' ). Instead of having a switch in the rulesets, I pretty much brought it in the template. I'd need to get back to it, but a more elegant solution would be to store the icon mapping in a unified table. This was just a minimal working alternative with icons actually in the HTML template and not only in the CSS.

I'd rather use a mapping, as suggested in my previous comment. And the mapping can be shared between extension and dependency icons.

manuelleduc · 2024-12-17T08:42:16Z

...re/xwiki-platform-web/xwiki-platform-web-templates/src/main/resources/templates/extension.vm

@@ -576,6 +576,14 @@ $namespace##
  </dd>
 #end

+#macro (spinner)


Duplicate of

xwiki-platform/xwiki-platform-core/xwiki-platform-export/xwiki-platform-export-pdf/xwiki-platform-export-pdf-ui/src/main/resources/XWiki/PDFExport/AdminSection.xml

Line 90 in be3b947

#macro (spinner)

If this is a common need, it's better to solve the code duplication, and to provide this as an actual API.
Meaning with a documented since version, and mentioned in the release notes.

👍

TODO provide this spinner as a macro in https://github.com/xwiki/xwiki-platform/blob/29111f02b38e529f44ff88aaf4ec67af92fcf3df/xwiki-platform-core/xwiki-platform-web/xwiki-platform-web-templates/src/main/resources/templates/macros.vm (which from my experience is the main place where we provide API-ish velocimacros).

* Improve code quality Co-authored-by: Manuel Leduc <manuel.leduc@xwiki.com>

mflorea · 2024-12-18T10:45:42Z

...re/xwiki-platform-web/xwiki-platform-web-templates/src/main/resources/templates/extension.vm

+    #if($!dependencyStatus == 'core' ||
+      $!dependencyStatus == 'remote-core' ||
+      $!dependencyStatus == 'remote-core-incompatible')
+      $services.icon.renderHTML('cog')
+    #end
+    #if($!dependencyStatus == 'installed' ||
+      $!dependencyStatus == 'remote-installed' ||
+      $!dependencyStatus == 'installed-invalid' ||
+      $!dependencyStatus == 'remote-installed-invalid' ||
+      $!dependencyStatus == 'installed-dependency' ||
+      $!dependencyStatus == 'remote-installed-dependency' ||
+      $!dependencyStatus == 'remote-installed-incompatible')
+      $services.icon.renderHTML('plugin')
+    #end
+    #if($!dependencyStatus == 'remote')
+      $services.icon.renderHTML('world')
+    #end
+    #if($!dependencyStatus == 'loading')
+      #spinner()
+    #end
+    #if($!dependencyStatus == 'unknown')
+      $services.icon.renderHTML('error')
+    #end


I'd rather use a mapping between dependency state and icon:

Suggested change

#if($!dependencyStatus == 'core' ||

$!dependencyStatus == 'remote-core' ||

$!dependencyStatus == 'remote-core-incompatible')

$services.icon.renderHTML('cog')

#end

#if($!dependencyStatus == 'installed' ||

$!dependencyStatus == 'remote-installed' ||

$!dependencyStatus == 'installed-invalid' ||

$!dependencyStatus == 'remote-installed-invalid' ||

$!dependencyStatus == 'installed-dependency' ||

$!dependencyStatus == 'remote-installed-dependency' ||

$!dependencyStatus == 'remote-installed-incompatible')

$services.icon.renderHTML('plugin')

#end

#if($!dependencyStatus == 'remote')

$services.icon.renderHTML('world')

#end

#if($!dependencyStatus == 'loading')

#spinner()

#end

#if($!dependencyStatus == 'unknown')

$services.icon.renderHTML('error')

#end

#set ($dependencyIcons = {

'core': 'cog',

...,

'installed': 'plugin',

...,

'remote': 'world',

'unknown': 'error'

})

#set ($dependencyIcon = $dependencyIcons.get($dependencyStatus))

#if ($dependencyIcon)

$services.icon.renderHTML($dependencyIcon)

#elseif ($dependencyStatus == 'loading')

#spinner()

#end

mflorea · 2024-12-18T10:48:38Z

...re/xwiki-platform-web/xwiki-platform-web-templates/src/main/resources/templates/extension.vm

@@ -1038,6 +1069,24 @@ $namespace##
    </div>
    <div class="extension-header">
      <h2 class="extension-title">
+        #if($!extensionStatus == 'core' ||


I'd rather use a mapping, as suggested in my previous comment. And the mapping can be shared between extension and dependency icons.

mflorea · 2024-12-18T10:49:03Z

...rm-web/xwiki-platform-web-war/src/main/webapp/resources/uicomponents/extension/extension.css

+}
+
+/* Style the icon at the start of the title. */
+.extension-title > *:first-child{


Suggested change

.extension-title > *:first-child{

.extension-title > *:first-child {

What if the icon is missing? The code you added to extension.vm allows this. We need to either choose a default icon or output some empty DIV when the extension / dependency state is not mapped to an icon. Or, even cleaner, add an icon wrapper (.extension-icon) and use that in the CSS.

mflorea · 2024-12-18T10:54:12Z

...rm-web/xwiki-platform-web-war/src/main/webapp/resources/uicomponents/extension/extension.css

  line-height: 1.4em;
 }
+
+.dependency-item > *:first-child {
+  margin-right: .3em;


Same as above, we need to make sure there's always an icon, otherwise this rule will target a different element.

mflorea · 2024-12-18T10:59:39Z

...orm-web/xwiki-platform-web-war/src/main/webapp/resources/uicomponents/extension/extension.js

@@ -495,6 +495,12 @@ XWiki.ExtensionBehaviour = Class.create({
      parameters : formData,
      onCreate : function() {
        dependency.removeClassName('extension-item-unknown').addClassName('extension-item-loading');
+        // Remove the unknown icon.
+        dependency.removeChild(dependency.childNodes[1]);


This is too fragile. I'd rather add an icon wrapper (.extension-icon) and look for it.

mflorea · 2024-12-18T11:05:23Z

...orm-web/xwiki-platform-web-war/src/main/webapp/resources/uicomponents/extension/extension.js

+        // Add the load icon
+        const loadPath = '$xwiki.getSkinFile("icons/xwiki/spinner.gif")';
+        const loadIcon = '<img  src="'+ loadPath + '" alt=""/>';
+        dependency.update(loadIcon + dependency.innerHTML);


I'd rather use https://developer.mozilla.org/en-US/docs/Web/API/Element/replaceWith to replace the unknown icon with the loading icon.

mflorea · 2024-12-18T11:08:08Z

...orm-web/xwiki-platform-web-war/src/main/webapp/resources/uicomponents/extension/extension.js

+        // Add the load icon
+        const loadPath = '$xwiki.getSkinFile("icons/xwiki/spinner.gif")';
+        const loadIcon = '<img  src="'+ loadPath + '" alt=""/>';


This doesn't match the behavior of the spinner Velocity macro. A possible solution is to always output the spinner from extension.vm, but hidden by default. It would be made visible by the extension-item-loading CSS class, which means no change in needed in this JavaScript file.

Sereza7 added the backport stable-15.10.x Used for automatic backport to 15.10.x branch. label Jan 25, 2024

Sereza7 added the backport stable-16.4.x label Oct 31, 2024

Sereza7 assigned manuelleduc Oct 31, 2024

Sereza7 removed the backport stable-15.10.x Used for automatic backport to 15.10.x branch. label Oct 31, 2024

manuelleduc self-requested a review December 17, 2024 08:27

manuelleduc reviewed Dec 17, 2024

View reviewed changes

...orm-web/xwiki-platform-web-war/src/main/webapp/resources/uicomponents/extension/extension.js Outdated Show resolved Hide resolved

manuelleduc reviewed Dec 17, 2024

View reviewed changes

manuelleduc added the backport stable-16.10.x label Dec 17, 2024

tmortagne requested a review from mflorea December 17, 2024 08:59

XWIKI-14342: Make Extension Manager use IconThemes

69424e0

* Improve code quality Co-authored-by: Manuel Leduc <manuel.leduc@xwiki.com>

Sereza7 marked this pull request as draft December 17, 2024 16:50

mflorea requested changes Dec 18, 2024

View reviewed changes

Sereza7 removed the backport stable-16.4.x label Jan 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XWIKI-14342: Make Extension Manager use IconThemes #2823

XWIKI-14342: Make Extension Manager use IconThemes #2823

Sereza7 commented Jan 25, 2024

manuelleduc Dec 17, 2024

Sereza7 Dec 17, 2024

Sereza7 Dec 17, 2024

manuelleduc Dec 17, 2024

tmortagne Dec 17, 2024

Sereza7 Dec 17, 2024

mflorea Dec 18, 2024

manuelleduc Dec 17, 2024 •

edited

Loading

manuelleduc Dec 17, 2024 •

edited

Loading

Sereza7 Dec 17, 2024

mflorea Dec 18, 2024

mflorea Dec 18, 2024

mflorea Dec 18, 2024

mflorea Dec 18, 2024

mflorea Dec 18, 2024

mflorea Dec 18, 2024

mflorea Dec 18, 2024

	.extension-title > *:first-child{
	.extension-title > *:first-child {

XWIKI-14342: Make Extension Manager use IconThemes #2823

Are you sure you want to change the base?

XWIKI-14342: Make Extension Manager use IconThemes #2823

Conversation

Sereza7 commented Jan 25, 2024

Jira URL

Changes

Description

Clarifications

Screenshots & Video

Executed Tests

Expected merging strategy

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuelleduc Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

manuelleduc Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuelleduc Dec 17, 2024 •

edited

Loading

manuelleduc Dec 17, 2024 •

edited

Loading