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.

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

Add support for alternative tagging of the presets. #126

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Sarabjeet108
Copy link

@Sarabjeet108 Sarabjeet108 commented Aug 10, 2023

Would it be better if we create two separate List for storing PresetLink objects with alternative="true" and the ones that don't have an alternative tag like presets under the "Edit also" section?

The current implementation depends on the order in which preset_link is defined in the XML file. It does not work if the preset_link with the alternative attribute is defined before the non-alternative ones or between them.

@tsmock

@tsmock
Copy link

tsmock commented Aug 15, 2023

It would probably be preferable to have two separate lists. We could sort based off of the alternative tag, but then we start getting into changing the order of the alternatives. This is probably undesirable, if the preset author put the preferred alternative first.

@Sarabjeet108
Copy link
Author

I added lists to store the PresetLink objects. Is there a need to sort the tags in the list?

@simonpoole
Copy link

In a follow up to @tsmock comment: from a functional pov an "alternative preset" is different than a "linked preset". The later offers additional tags that might be added, the former replaces the current tagging with an alternative.

tl;dr the alternative preset links need a separate heading/label that makes the above clear (the "Edit also..." is very confusing here).

@@ -288,7 +288,7 @@
<complexType name="preset_link">
<annotation>
<documentation><![CDATA[
Adds a link to an other preset with a label on top. The preset_name attribute is required, text to override the label (default is "Edit also …") and text_context are optional. A sequence of <preset_link /> without text or a identical text value are grouped below one label. Watch out for presets with identical name as it is not predictable to which preset the link will lead to, see #12716.
Adds a link to an other preset with a label on top. The preset_name attribute is required. alternative, text to override the label (default is "Edit also …") and text_context are optional. A sequence of <preset_link /> without text or a identical text value are grouped below one label. Watch out for presets with identical name as it is not predictable to which preset the link will lead to, see #12716.
Copy link

Choose a reason for hiding this comment

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

I'm not quite certain what you are trying to say/change here.

Comment on lines +71 to +80
initializeLocaleText(tr("Additional tags to edit"));
return new JLabel(locale_text);
}

/**
* Creates a label to be inserted above the alternative presets
* @return a label
*/
public JLabel createAlternativeLabel() {
initializeLocaleText(tr("Similar but different tags"));
Copy link

Choose a reason for hiding this comment

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

We only use createLabel in one location. These two methods should probably be consolidated.

Suggested change
initializeLocaleText(tr("Additional tags to edit"));
return new JLabel(locale_text);
}
/**
* Creates a label to be inserted above the alternative presets
* @return a label
*/
public JLabel createAlternativeLabel() {
initializeLocaleText(tr("Similar but different tags"));
if (!alternative) {
initializeLocaleText(tr("Additional tags to edit"));
return new JLabel(locale_text);
}
initializeLocaleText(tr("Similar but different tags"));

Comment on lines +442 to +457
if (!alternativeTags.isEmpty()) {
PresetLink link = new PresetLink();
itemPanel.add(link.createAlternativeLabel(), GBC.eol().insets(0, 8, 0, 0));
for (PresetLink links : alternativeTags) {
links.addToPanel(itemPanel, itemGuiSupport);
p.hasElements = true;
}
}

if (!editAlsoTags.isEmpty()) {
PresetLink link = new PresetLink();
itemPanel.add(link.createLabel(), GBC.eol().insets(0, 8, 0, 0));
for (PresetLink links : editAlsoTags) {
links.addToPanel(itemPanel, itemGuiSupport);
p.hasElements = true;
}
Copy link

Choose a reason for hiding this comment

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

If you consolidate the createLabel/createAlternativeLabel, this could be done like so:

Suggested change
if (!alternativeTags.isEmpty()) {
PresetLink link = new PresetLink();
itemPanel.add(link.createAlternativeLabel(), GBC.eol().insets(0, 8, 0, 0));
for (PresetLink links : alternativeTags) {
links.addToPanel(itemPanel, itemGuiSupport);
p.hasElements = true;
}
}
if (!editAlsoTags.isEmpty()) {
PresetLink link = new PresetLink();
itemPanel.add(link.createLabel(), GBC.eol().insets(0, 8, 0, 0));
for (PresetLink links : editAlsoTags) {
links.addToPanel(itemPanel, itemGuiSupport);
p.hasElements = true;
}
for (List<PresetLink> linkList : Arrays.asList(alternativeTags, editAlsoTags)) {
if (!linkList.isEmpty()) {
PresetLink link = new PresetLink();
itemPanel.add(link.createLabel(), GBC.eol().insets(0, 8, 0, 0));
for (PresetLink links : linkList) {
links.addToPanel(itemPanel, itemGuiSupport);
p.hasElements = true;
}
}
}

Copy link
Author

Choose a reason for hiding this comment

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

link.isAlternative() is always false, therefore same label is being created for both preset alternative and editalso tags. We may want to create label based on the links of the linkedList

}
}
if (i.addToPanel(itemPanel, itemGuiSupport)) {
if (!(i instanceof PresetLink) && (i.addToPanel(itemPanel, itemGuiSupport))) {
Copy link

Choose a reason for hiding this comment

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

You might want to add a comment for this (e.g. "Not adding preset links to panel here since we want to order alternative tags and tags that users may want to use in addition").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants