XWIKI-19383: Display a title on createlink #256
Conversation
|
PR Changes:
NOTE: This commit needs to be merged with/before xwiki/xwiki-platform#2104 Default behavior is to just use the name of the page to create as the title. |
.../main/java/org/xwiki/rendering/internal/renderer/DefaultPageAttachmentURITitleGenerator.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public String generateCreateTitle(ResourceReference reference) | ||
| { | ||
| return "Create page: " + reference.getReference(); |
There was a problem hiding this comment.
There should be a translation used here no?
There was a problem hiding this comment.
This is the default behavior, with the rendering module on its own. AFAIK there is no access to translations here.
See the full implementation in xwiki-platform.
There was a problem hiding this comment.
In this case, we should discuss a way to allow xwiki platform to inject the a translation here. Having some text in an unknown language is not going to help accessibility for non-English speaking users.
There was a problem hiding this comment.
ok, iiuc in relation with xwiki/xwiki-platform#2104, XWikiDocumentURITitleGenerator will be used instead of the default implem. And in this case the returned title is translated.
If that's correct might be worth adding some comments :)
There was a problem hiding this comment.
This behavior is overwritten by the xwiki platform class I linked above.
I followed the pattern used so far when we needed translations in xwiki-rendering.
See Michael's comment on the jira issue.
Other similar uses of this pattern can be found here and here.
I thought it would be better to have, in all cases, a readable english title instead of just a value that could hold anything (possibly complete gibberish). We could remove this default formatting to just return reference.getReference() but I don't think it'd be best.
...ndering-api/src/main/java/org/xwiki/rendering/renderer/reference/link/URITitleGenerator.java
Outdated
Show resolved
Hide resolved
| * @since 15.2-RC1 | ||
| */ | ||
| @Component | ||
| @Named("doc") |
There was a problem hiding this comment.
AFAICS this is the default component, so the one to be used "by default", so I don't think it should have a @Named.
Now if it's not the default component, then you should probably rename the class.
There was a problem hiding this comment.
This is the default component used when xwiki-rendering runs on its own. This component needs to have an hint that corresponds to the kind of object I want to process. It's not the default on the resourceType axis.
I'm fixing the names of both the classes to make it clearer that the resourceType is document 👍
I replicated the pattern used here.
...ain/java/org/xwiki/rendering/internal/renderer/xhtml/link/AbstractXHTMLLinkTypeRenderer.java
Outdated
Show resolved
Hide resolved
...ain/java/org/xwiki/rendering/internal/renderer/xhtml/link/AbstractXHTMLLinkTypeRenderer.java
Outdated
Show resolved
Hide resolved
|
|
||
| /** | ||
| * Generate Resource Reference titles for URIs. For example an implementation for MAILTO URIs would remove the scheme | ||
| * part and the query string part. |
There was a problem hiding this comment.
So you indicated below:
// Look for a component implementing URITitleGenerator with a role hint matching the link scheme.
This should be specified in this documentation here, as it's how the implementation should be named.
...pi/src/main/java/org/xwiki/rendering/internal/renderer/DefaultDocumentURITitleGenerator.java
Outdated
Show resolved
Hide resolved
...pi/src/main/java/org/xwiki/rendering/internal/renderer/DefaultDocumentURITitleGenerator.java
Outdated
Show resolved
Hide resolved
...ain/java/org/xwiki/rendering/internal/renderer/xhtml/link/AbstractXHTMLLinkTypeRenderer.java
Outdated
Show resolved
Hide resolved
...ain/java/org/xwiki/rendering/internal/renderer/xhtml/link/AbstractXHTMLLinkTypeRenderer.java
Show resolved
Hide resolved
...ain/java/org/xwiki/rendering/internal/renderer/xhtml/link/AbstractXHTMLLinkTypeRenderer.java
Outdated
Show resolved
Hide resolved
...ain/java/org/xwiki/rendering/internal/renderer/xhtml/link/DocumentXHTMLLinkTypeRenderer.java
Outdated
Show resolved
Hide resolved
...ain/java/org/xwiki/rendering/internal/renderer/xhtml/link/DocumentXHTMLLinkTypeRenderer.java
Show resolved
Hide resolved
...ain/java/org/xwiki/rendering/internal/renderer/xhtml/link/DocumentXHTMLLinkTypeRenderer.java
Outdated
Show resolved
Hide resolved
...ain/java/org/xwiki/rendering/internal/renderer/xhtml/link/DocumentXHTMLLinkTypeRenderer.java
Outdated
Show resolved
Hide resolved
|
PR Changes:
|
| * Used to generate a link title. | ||
| */ | ||
| @Inject | ||
| private URITitleGenerator defaultTitleGenerator; |
There was a problem hiding this comment.
This is not going to work since there's no default implementation.
| } catch (Exception e) { | ||
| logger.warn("Error while loading component for generating URI title: [{}]", | ||
| ExceptionUtils.getRootCauseMessage(e)); | ||
| logger.debug("Full stack trace: ", e); |
There was a problem hiding this comment.
I would write:
String message = "Error while loading component for generating URI title";
if (logger.isDebugEnabled()) {
logger.debug(message, e);
} else {
logger.warn(String.format("%s: [{}]", message), ExceptionUtils.getRootCauseMessage(e));
}
There was a problem hiding this comment.
In addition you should also log the reference and explain which component we're talking about.
| */ | ||
| @Role | ||
| @Unstable | ||
| public interface URITitleGenerator |
There was a problem hiding this comment.
I don't understand why it's a URI. I'd name this: ResourceReferenceTitleGenerator.
EDIT: I've just realized that the implementation is only for wanted links and not to generate titles in general. Thus, I'd name this WantedLinkTitleGenerator.
| * @version $Id$ | ||
| * @since 15.2RC1 | ||
| */ | ||
| @Component(hints = {"doc", "page"}) |
There was a problem hiding this comment.
Error here, should not contain any hint.
| * The implementations should be named according to the kind of reference they process. | ||
| * | ||
| * @version $Id$ | ||
| * @since 15.2RC1 |
There was a problem hiding this comment.
Needs to be updated to 15.3RC1 now ;)
| getXHTMLWikiPrinter().printXMLStartElement(SPAN, new String[][] { { CLASS, "wikigeneratedlinkcontent" } }); | ||
| getXHTMLWikiPrinter().printXMLStartElement(SPAN, new String[][] | ||
| { | ||
| { CLASS, "wikigeneratedlinkcontent" } |
There was a problem hiding this comment.
I wouldn't change the formatting (unless there's a good reason)
| import org.xwiki.rendering.renderer.reference.link.URITitleGenerator; | ||
|
|
||
| /** | ||
| * Generate link titles for DOC URIs. |
There was a problem hiding this comment.
Improve the javadoc and explain what it does (use the reference as the title) and explain that it's supposed to be overridden to provide better titles.
|
Missing some test (which explains why the code is buggy - no default implementation - but it was not noticed). Have you built with |
| private URITitleGenerator getTitleGenerator(ResourceReference reference) | ||
| { | ||
| URITitleGenerator titleGenerator = this.defaultTitleGenerator; | ||
| if (this.componentManager.hasComponent(URITitleGenerator.class, reference.getType().getScheme())) { |
There was a problem hiding this comment.
I don't see why need this as the implementation is fixed and doesn't depend on the resource reference type.
| titleGenerator = this.componentManager.getInstance(WantedLinkTitleGenerator.class, | ||
| reference.getType().getScheme()); | ||
| } catch (Exception e) { | ||
| String message = "Could not find a [WantedLinkTitleGenerator] component to generate the wanted " |
There was a problem hiding this comment.
you should use WantedLinkTitleGenerator.class.getName() instead of hardcoding it IMO.
| WantedLinkTitleGenerator titleGenerator = this.defaultTitleGenerator; | ||
| try { | ||
| titleGenerator = this.componentManager.getInstance(WantedLinkTitleGenerator.class, | ||
| reference.getType().getScheme()); |
There was a problem hiding this comment.
As far as I can see, this won't fallback to the default. I think a fallback to the default should be implemented, otherwise I'm not sure why there is a default implementation at all.
* Added a title attribute to the generated wikicreatelink span
* Fixed wrong since javadocs tags
* Added an hint on the URITitleGenerator component
* Fixed the hint of the URITitleGenerator implementation
* Refactored name * Fixed hint to match DocumentXHTMLLinkTypeRenderer. * Moved behavior from the AbstractXHTMLLinkTypeRenderer to DocumentXHTMLLinkTypeRenderer * Fixed since annotation format * Marked the API as unstable * Fixed comments.
* Inject logger Co-authored-by: Manuel Leduc <[email protected]>
* Fixed code style * Used logger injection * Fixed a constant name
* Added a comment to DefaultDocumentURITileGenerator
* Fixed types and names TODO: * Fix tests * Check DocumentXHTMLLinkTypeRenderer.java L105
* Fixed integration tests
* Replaced a hard-coded string
* Updated versions
Jira: https://jira.xwiki.org/browse/XWIKI-19383
PR Changes: