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.

Sign up for GitHub

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

XWIKI-14342: Make Extension Manager use IconThemes #2823

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

XWIKI-14342: Make Extension Manager use IconThemes #2823

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b316699
XWIKI-14342: Make Extension Manager use IconThemes
Sereza7 Jan 25, 2024
69424e0
XWIKI-14342: Make Extension Manager use IconThemes
Sereza7 Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions ...xwiki-platform-web/xwiki-platform-web-templates/src/main/resources/templates/extension.vm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,6 +576,14 @@ $namespace##
</dd>
#end

#macro (spinner)
Copy link
Contributor

@manuelleduc manuelleduc Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

@manuelleduc manuelleduc Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

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).

#if ($services.icon.currentIconSetName == 'Font Awesome')
<span class="fa fa-spinner fa-spin"></span>
#else
<img src="$escapetool.xml($xwiki.getSkinFile('icons/xwiki/spinner.gif'))"/>
#end
#end

#**
* NOTE: We explicitly overwrite the $extensionNamespace global variable because we want the dependency status to be
* determined for the given namespace. See #determineExtensionStatus() macro.
Expand Down Expand Up @@ -613,6 +621,29 @@ $namespace##
#set ($dependencyName = "<a href=""$dependencyURL"" class=""extension-link"">#displayExtensionName($dependencyExtension)</a>")
#end
<div class="dependency-item extension-item-$dependencyStatus">
#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
Comment on lines +624 to +646
Copy link
Member

Choose a reason for hiding this comment

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

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

<span class="extension-name">${dependencyName}</span><span class="extension-version">$!dependencyVersion</span>
#if ($extensionNamespace.startsWith('wiki:'))
<span class="extension-namespace">$services.localization.render('extensions.info.dependency.wiki', ["#wikiHomePageLink($extensionNamespace)"])</span>
Expand Down Expand Up @@ -1038,6 +1069,24 @@ $namespace##
</div>
<div class="extension-header">
<h2 class="extension-title">
#if($!extensionStatus == 'core' ||
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

$!extensionStatus == 'remote-core')
$services.icon.renderHTML('cog')
#end
#if($!extensionStatus == 'installed' ||
$!extensionStatus == 'remote-installed' ||
$!extensionStatus == 'installed-dependency' ||
$!extensionStatus == 'remote-installed-dependency' ||
$!extensionStatus == 'installed-invalid' ||
$!extensionStatus == 'remote-installed-invalid')
$services.icon.renderHTML('plugin')
#end
#if($!extensionStatus == 'remote')
$services.icon.renderHTML('world')
#end
#if($!extensionStatus == 'loading')
#spinner()
#end
<span class="extension-name">#displayExtensionName($extension)</span>
<span class="extension-version">$escapetool.xml($extension.id.version)</span>
#if ($extensionStatusMessage)
Expand Down
38 changes: 12 additions & 26 deletions ...web/xwiki-platform-web-war/src/main/webapp/resources/uicomponents/extension/extension.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,12 @@
border: none;
background: none no-repeat scroll 0.2em 0.3em transparent;
margin: 0 0 0 -1.8em;
padding: 2px 0 2px 1.8em;
padding: 2px 0 2px 0;
}

/* Style the icon at the start of the title. */
.extension-title > *:first-child{
Copy link
Member

Choose a reason for hiding this comment

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

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.

margin-right: .8em;
}

.extension-version, .extension-namespace {
Expand Down Expand Up @@ -170,37 +175,18 @@ ul.dependencies li:hover {
background: transparent none no-repeat left;
border-bottom: 1px dotted #E8E8E8;
border-right: 7px solid $theme.pageContentBackgroundColor;
padding: 0.3em 0 0.3em 20px;
padding: 0.3em 0 0.3em 0;
position: relative;
}
.dependency-item {
line-height: 1.4em;
}

.dependency-item > *:first-child {
margin-right: .3em;
Copy link
Member

Choose a reason for hiding this comment

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

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

}

.dependency-item .extension-status {
margin: 0;
}
.extension-item-core .extension-title, .extension-item-remote-core .extension-title,
.dependency-item.extension-item-core, .dependency-item.extension-item-remote-core, .dependency-item.extension-item-remote-core-incompatible {
background-image: url("$xwiki.getSkinFile('icons/silk/cog.png')");
}
.extension-item-installed .extension-title, .extension-item-remote-installed .extension-title,
.extension-item-installed-dependency .extension-title, .extension-item-remote-installed-dependency .extension-title,
.extension-item-installed-invalid .extension-title, .extension-item-remote-installed-invalid .extension-title,
.dependency-item.extension-item-installed, .dependency-item.extension-item-remote-installed,
.dependency-item.extension-item-installed-invalid, .dependency-item.extension-item-remote-installed-invalid,
.dependency-item.extension-item-installed-dependency, .dependency-item.extension-item-remote-installed-dependency,
.dependency-item.extension-item-remote-installed-incompatible {
background-image: url("$xwiki.getSkinFile('icons/silk/plugin.png')");
}
.extension-item-remote .extension-title, .dependency-item.extension-item-remote {
background-image: url("$xwiki.getSkinFile('icons/silk/world.png')");
}
.extension-item-loading .extension-title, .dependency-item.extension-item-loading {
background-image: url("$xwiki.getSkinFile('icons/xwiki/spinner.gif')");
}
.dependency-item.extension-item-unknown {
background-image: url("$xwiki.getSkinFile('icons/silk/plugin_error.png')");
}

.extension-item-core,
.extension-item-installed-dependency {
Expand Down
6 changes: 6 additions & 0 deletions ...-web/xwiki-platform-web-war/src/main/webapp/resources/uicomponents/extension/extension.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

dependency.removeChild(dependency.childNodes[1]);
Copy link
Member

Choose a reason for hiding this comment

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

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

// Add the load icon
const loadPath = '$xwiki.getSkinFile("icons/xwiki/spinner.gif")';
const loadIcon = '<img src="'+ loadPath + '" alt=""/>';
Comment on lines +500 to +502
Copy link
Member

Choose a reason for hiding this comment

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

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.

dependency.update(loadIcon + dependency.innerHTML);
Copy link
Member

Choose a reason for hiding this comment

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

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

},
onSuccess : function(response) {
// Update the dependency if it's still attached to the document.
Expand Down