Skip to content

Comments

XWIKI-19383: Display a title on createlink#256

Draft
Sereza7 wants to merge 15 commits intoxwiki:masterfrom
Sereza7:XWIKI-19383
Draft

XWIKI-19383: Display a title on createlink#256
Sereza7 wants to merge 15 commits intoxwiki:masterfrom
Sereza7:XWIKI-19383

Conversation

@Sereza7
Copy link
Contributor

@Sereza7 Sereza7 commented Mar 6, 2023

Jira: https://jira.xwiki.org/browse/XWIKI-19383
PR Changes:

  • Added a title to the wikicreatelink spans rendered from user generated content
  • Added a default behavior for standalone use of xwiki-rendering, and an endpoint to complete this behaviour from xwiki-platform.

@Sereza7 Sereza7 mentioned this pull request Mar 6, 2023
Draft
@Sereza7 Sereza7 marked this pull request as ready for review March 16, 2023 16:15
@Sereza7
Copy link
Contributor Author

Sereza7 commented Mar 16, 2023

PR Changes:

  • Solved the mismatch in hints for the URITitleGenerator implementation

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.

surli
surli reviewed Mar 17, 2023
View reviewed changes
.../main/java/org/xwiki/rendering/internal/renderer/DefaultPageAttachmentURITitleGenerator.java Outdated
@Override
public String generateCreateTitle(ResourceReference reference)
{
return "Create page: " + reference.getReference();
Copy link
Member

@surli surli Mar 17, 2023

Choose a reason for hiding this comment

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

There should be a translation used here no?

Copy link
Contributor Author

@Sereza7 Sereza7 Mar 20, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@manuelleduc manuelleduc Mar 20, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@manuelleduc manuelleduc Mar 20, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Sereza7 Sereza7 Mar 20, 2023

Choose a reason for hiding this comment

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

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.

surli
surli reviewed Mar 17, 2023
View reviewed changes
.../main/java/org/xwiki/rendering/internal/renderer/DefaultPageAttachmentURITitleGenerator.java Outdated
* @since 15.2-RC1
*/
@Component
@Named("doc")
Copy link
Member

@surli surli Mar 17, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Sereza7 Sereza7 Mar 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

surli
surli reviewed Mar 17, 2023
View reviewed changes
...ndering-api/src/main/java/org/xwiki/rendering/renderer/reference/link/URITitleGenerator.java Outdated

/**
* Generate Resource Reference titles for URIs. For example an implementation for MAILTO URIs would remove the scheme
* part and the query string part.
Copy link
Member

@surli surli Mar 17, 2023

Choose a reason for hiding this comment

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

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.

@Sereza7
Copy link
Contributor Author

Sereza7 commented Mar 20, 2023

PR Changes:

  • Refactored name
  • Fixed hint to match DocumentXHTMLLinkTypeRenderer.
  • Moved behavior from the AbstractXHTMLLinkTypeRenderer to DocumentXHTMLLinkTypeRenderer
  • Marked the API as unstable
  • Fixed code style
  • Improved comments

vmassol
vmassol reviewed Mar 29, 2023
View reviewed changes
...ain/java/org/xwiki/rendering/internal/renderer/xhtml/link/DocumentXHTMLLinkTypeRenderer.java Outdated
* Used to generate a link title.
*/
@Inject
private URITitleGenerator defaultTitleGenerator;
Copy link
Member

@vmassol vmassol Mar 29, 2023

Choose a reason for hiding this comment

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

This is not going to work since there's no default implementation.

@vmassol
Copy link
Member

vmassol commented Mar 29, 2023 •
edited
Loading

Missing some test (which explains why the code is buggy - no default implementation - but it was not noticed). Have you built with -Pquality?

@vmassol
Copy link
Member

vmassol commented Mar 29, 2023 •
edited
Loading

Have you tested that https://github.com/xwiki/xwiki-platform/blob/9330fea36561122e2a7ce38bec6fb2c6ccde48a6/xwiki-platform-core/xwiki-platform-legacy/xwiki-platform-legacy-oldcore/src/test/java/com/xpn/xwiki/doc/XWikiDocumentTest.java#L280 passes?

Same question for https://github.com/xwiki/xwiki-rendering/blob/0786014ad4e09b5e64c22c074297461c92d2fb81/xwiki-rendering-integration-tests/src/test/resources/wiki/link/links6.test and others.

vmassol
vmassol reviewed Mar 29, 2023
View reviewed changes
...ain/java/org/xwiki/rendering/internal/renderer/xhtml/link/DocumentXHTMLLinkTypeRenderer.java Outdated
private URITitleGenerator getTitleGenerator(ResourceReference reference)
{
URITitleGenerator titleGenerator = this.defaultTitleGenerator;
if (this.componentManager.hasComponent(URITitleGenerator.class, reference.getType().getScheme())) {
Copy link
Member

@vmassol vmassol Mar 29, 2023

Choose a reason for hiding this comment

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

I don't see why need this as the implementation is fixed and doesn't depend on the resource reference type.

@Sereza7 Sereza7 marked this pull request as draft March 31, 2023 08:52
vmassol
vmassol reviewed Apr 20, 2023
View reviewed changes
...ain/java/org/xwiki/rendering/internal/renderer/xhtml/link/DocumentXHTMLLinkTypeRenderer.java Outdated
titleGenerator = this.componentManager.getInstance(WantedLinkTitleGenerator.class,
reference.getType().getScheme());
} catch (Exception e) {
String message = "Could not find a [WantedLinkTitleGenerator] component to generate the wanted "
Copy link
Member

@vmassol vmassol Apr 20, 2023

Choose a reason for hiding this comment

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

you should use WantedLinkTitleGenerator.class.getName() instead of hardcoding it IMO.

michitux
michitux reviewed May 26, 2023
View reviewed changes
...ain/java/org/xwiki/rendering/internal/renderer/xhtml/link/DocumentXHTMLLinkTypeRenderer.java
WantedLinkTitleGenerator titleGenerator = this.defaultTitleGenerator;
try {
titleGenerator = this.componentManager.getInstance(WantedLinkTitleGenerator.class,
reference.getType().getScheme());
Copy link
Contributor

@michitux michitux May 26, 2023

Choose a reason for hiding this comment

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

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.

Sereza7 and others added 10 commits April 5, 2024 11:14
@Sereza7
XWIKI-19383: Display a title on createlink
fa77dcb 
* Added a title attribute to the generated wikicreatelink span
@Sereza7
XWIKI-19383: Display a title on createlink
f3b222e 
* Fixed wrong since javadocs tags
@Sereza7
XWIKI-19383: Display a title on createlink
84de641 
* Added an hint on the URITitleGenerator component
@Sereza7
XWIKI-19383: Display a title on createlink
4e29001 
* Fixed the hint of the URITitleGenerator implementation
@Sereza7
XWIKI-19383: Display a title on createlink
a3689eb 
* 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.
@Sereza7 @manuelleduc
XWIKI-19383: Display a title on createlink
e275325 
* Inject logger

Co-authored-by: Manuel Leduc <manuel.leduc@xwiki.com>
@Sereza7
XWIKI-19383: Display a title on createlink
de2d1e7 
* Fixed code style
* Used logger injection
* Fixed a constant name
@Sereza7
XWIKI-19383: Display a title on createlink
3cbda64 
* Added a comment to DefaultDocumentURITileGenerator
@Sereza7
XWIKI-19383: Display a title on createlink
42255e7 
* Fixed types and names

TODO:
* Fix tests
* Check DocumentXHTMLLinkTypeRenderer.java L105
@Sereza7
XWIKI-19383: Display a title on createlink
17a58a6 
* Fixed integration tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michitux michitux michitux left review comments

@manuelleduc manuelleduc manuelleduc left review comments

@vmassol vmassol vmassol left review comments

@surli surli surli left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Sereza7 @vmassol @michitux @manuelleduc @surli