[MGNLUI-3683] Make LinkField a pure-Vaadin field, move definition handling to factory Created: 30/Nov/15  Updated: 09/Feb/17  Resolved: 10/Dec/15

Status: Closed
Project: Magnolia UI
Component/s: forms
Affects Version/s: 5.4
Fix Version/s: 5.5

Type: Improvement Priority: Minor
Reporter: Mikaël Geljić Assignee: Ngoc Nguyenthanh
Resolution: Fixed Votes: 0
Labels: linkfield
Remaining Estimate: 0d
Time Spent: 2.5d
Original Estimate: 3d

Issue Links:
Cloners
clones MGNLUI-2954 LinkField shows preview section even ... Closed
Relates
relates to MGNLUI-4101 LinkFieldFactory ignores workspaceNam... Closed
relates to MGNLUI-3725 Reconsider JCR-specific code in LinkF... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Date of First Response:
Sprint: Saigon 43
Story Points: 3

 Description   

Just aligning on the field vs. factory pattern



 Comments   
Comment by Aleksandr Pchelintcev [ 15/Dec/15 ]

Please DO NOT INTEGRATE THIS ONE UNTIL VERSION 5.5

Comment by Philip Mundt [ 14/Dec/16 ]

I'm not very happy with the description of this ticket and the commit message itself I have to admit.

The change brought in by this ticket basically makes any workspaceName set on the info.magnolia.ui.form.field.converter.IdentifierToPathConverter useless and it's basically ignored. This is due to the LinkFieldFactory overriding any workspaceName given with what it finds in its own definition (as targetWorkspace). I agree that this makes sense to some extend but what about the workspaceName of IdentifierToPathConverter? Is it deprecated now?

Additionally, the logic involved

IdentifierToPathConverter converter = definition.getIdentifierToPathConverter();
if (converter != null) {
    converter.setWorkspaceName(definition.getTargetWorkspace());
}
linkField.setTextFieldConverter(converter);

could have been better, no (thinking about the null-check)?

Note: I found this only because of MGNLRSSAGG-211 and the N2B problem with property workspace which (rightly) couldn't be set. But the converter was still working...

Comment by Mikaël Geljić [ 14/Dec/16 ]

PR/commit state exactly what was done, and ticket description gives the rationale: definition handling was moved to the factory. 1-to-1. Really.
So no, worspaceName wasn't made useless by this ticket, it was useless before; and the logic was the same too, so I'm not surprised the converter was still working.

That said, I understand you were hoping for more from whoever touched this last.

Comment by Philip Mundt [ 14/Dec/16 ]

True, it was borked previously already. I created an issue and linked them, will adjust type of link.
Edit: actually, it has been like for a long, long time.

Generated at Mon Feb 12 09:09:05 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.