This reverts commit 950a5300dc.
Reason for revert: This broke several workflows. The reason is
that MWParameterPlaceholderPage & MWParameterSearchWidget both
hold references to the MWTemplateModel. This model is not
always the same. The dialog might be the same when a template
is edited multiple times. But the model might be a new one.
From this point on the MWParameterSearchWidget pulls data from
an outdated model.
Bug: T284636
Bug: T285571
Change-Id: I7b9ea8cab8f17705ec8020f07e3732da6ba0e73c
This does not revert commit 950a5300 but applies the most
minimal hotfix I could come up with.
The reason for the breakage is that MWParameterPlaceholderPage
& MWParameterSearchWidget both hold references to the
MWTemplateModel. This model is not always the same. The dialog
might be the same when a template is edited multiple times.
But the model might be a new one. From this point on the
MWParameterSearchWidget pulls data from an outdated model.
This extra check compares this model reference and creates a
new widget when it changed.
Bug: T284636
Bug: T285571
Change-Id: Ib3eca52bbff90ffbf56a257e3984adcbe02b310b
There is a codepath where `modelPromise` is undefined and
calling `modelPromise.then()` fails. This codepath implies
that the dialog is empty and there is nothing to update. We
can just close the dialog then.
I found this while debugging the actions in this dialog.
This happens when the dialog is empty (except for a
placeholder) but you submit it anyway. This is typically
not possible as the button is supposed to be disabled.
Still I think it's a good idea to make this code less
fragile.
The relevant code was introduced in Ibc2fc66 (2016).
Change-Id: Ia6b723548456c211b944a2320949bfc23b0afa16
These comments don't add any knowledge. The text is either
duplicated, or the method signatur says it already. Having
to read these comments just to realize that they don't give
any additional information is not helpful, even error-prone.
Change-Id: I014028b1e9311b831a22c37859b2130aed2e9539
Wait, what's going on here? This patch looks like it changes the
behavior of this code. But it doesn't. Here is what happened
before:
* Let's say a template contains 2 parameters, A and B.
* We don't know yet if these names are aliases.
* getParameterNames() returns [ "A", "B" ].
* extend() is called. The TemplateData documentation contains
the parameters "B" and "C". "C" does have an alias "A".
* extend() can't find "C" and adds it to the end, as if it's a
new parameter.
* extend() also iterates the aliases. For each alias it creates
a reference to the specification object. In this case a
reference from "A" to "C" is created.
* But "A" already exists. The position of "A" doesn't change,
but the specification now says it's an alias.
* getParameterNames() skips aliases. It skips "A" and instead
returns the new "C" from the end of the list.
This was the behavior before. It's unchanged, proven by the tests.
Change-Id: I04b8a14fbec7be5a1c4defabf92e94f694c1e638
The idea is to not actually store all these default values, but
fall back to the default only when needed.
Some more details:
* The only remaining property is ….name. The only reason to
have this property is to distinguish between aliases and
primary parameter names. This will be reworked in a later
patch.
* The description falls back to null because this is the
documented fallback, not undefined.
* The default value falls back to "", same as the auto-value.
Why not null you might ask. This is intentional. Both the
auto- and default value are effectively wikitext snippets,
while the example is a label in the VE UI.
Bug: T285483
Change-Id: I1be3cca18f9ad6fc1c16362b24633f7613f02539
This is done for two reasons:
1. It fixes the behavior of two methods in rare edge-case
situations. They aren't documented to return undefined.
2. It reduces the amount of stuff this class stores when it's
nothing but a default value anyway. Note this patch does this
for the template-level properties only. Another patch will do
the same for the parameter-level properties.
Bug: T285483
Change-Id: If2e4d56da1fa52e32dc94191f36d7dc6a1487829
This reflects much better how this method is meant to behave.
Note I will continue to remove documentation that doesn't
explain anything in addition to what the code already says.
Bug: T285483
Change-Id: I81fa8a5d9d0752f3aeac4015c9a27b50e054d4df
This patch also marks 2 methods as @private that are not and
should not be used outside of this class.
Bug: T285483
Change-Id: I8a8ffc4868a369b5c47068beb0e83f023872543d
This reverts the revert commit d47b95eb4a.
When no `paramOrder` is given, known parameters should appear in the
order returned from the TemplateData API.
Previously, when TemplateData was present but no paramOrder
specified, then the parameters would appear in alphabetical order as
"unknown" parameters. Now they will appear in the order listed in
TemplateData. This is similar to the fully-specified behavior when
paramOrder is present.
This will only affect the Visual Editor template dialog, and has no
effect on serialization.
Bug: T274545
Change-Id: If8315781572af688ea1c1b14b3694b828f076b4a
The results show that parameter order always follows the appearance
in the template invocation, regardless of `paramOrder`, whether the
parameters are aliased, or whether there are unknown params.
Bug: T285382
Change-Id: I76c6fe8f0a2482cf0856bbafd9f21ba9fc4919a4
This makes the code more readable and easier to reason about.
The ESLint rule responsible for this code style was removed
just recently.
Notes:
* I focus on classes that are relevant for what the WMDE team
does right now.
* I merge multiple `var` keywords only when the variables are
strongly connected.
* Caching the length in a for loop makes the code hard to
read, but not really faster when it's a trivial property
access anyway.
Bug: T284895
Change-Id: I621fed61d894a83dc95f58129bbe679d82b0f5f5
This reverts parts of I678bb24.
Brief history of this code:
2014: The dialog was designed to dynamically change the title.
There was never a tooltip.
2016: A change in OOjs changed the behavior in VE. Now the initial
title shows up as a tooltip. It never updates because VE
does not know about this. The tooltip does not match the
visible title.
2021: We revert to the behavior from 2014. We achieve this by
bypassing the codepath that creates the tooltip. This is why
….title.setLabel() is used instead of ….static.title.
Bug: T276568
Change-Id: I346a904881c3a63186d6a80afdaf717688bab42a
When no `paramOrder` is given, known parameters should appear in the
order returned from the TemplateData API.
Previously, when TemplateData was present but no paramOrder
specified, then the parameters would appear in alphabetical order as
"unknown" parameters. Now they will appear in the order listed in
TemplateData. This is similar to the fully-specified behavior when
paramOrder is present.
This will only affect the Visual Editor template dialog.
Bug: T274545
Change-Id: I32538de07641c288081042a41fe39eedfed7d939
Note that the tests expose a bug, getAllParametersOrdered fails to
list an unused parameter. Fixed in I32538de07641c.
Also, a minor fix to avoid an impossible template spec: paramOrder
must include all parameters.
Bug: T274545
Change-Id: Icfa7a765773d04ef05a76ecc09467305e311f6cb
