Skip to content

Act on the selected addon preset, not a typed name - #1980

Open
MichalLabuda wants to merge 2 commits into
Return-To-The-Roots:masterfrom
MichalLabuda:addon-presets-selection
Open

Act on the selected addon preset, not a typed name#1980
MichalLabuda wants to merge 2 commits into
Return-To-The-Roots:masterfrom
MichalLabuda:addon-presets-selection

Conversation

@MichalLabuda

Copy link
Copy Markdown
Contributor

Follow-up to #1951

Removed Delete button from Save Addon Preset window
Removed name field from Load Addon Preset window

Load and Delete resolved their target from the name field, with the list
selection only prefilling it. So editing the name after picking a preset
silently retargeted the action, an unmatched name raised "Preset Not Found",
and an empty one did nothing.

Both now act on the list selection and are disabled until a preset is picked.
That makes those states unrepresentable, so GetTargetFilePath(),
GetTargetFileOrNotify() and the "Preset Not Found" strings are gone.

A typed name is only needed for saving, and deleting belongs with browsing
presets rather than with saving one.

image image

Load and Delete resolved their target from the name field,
so editing the name after picking a preset silently retargeted the
action.

Removed the Delete button from the Save preset window.
Removed the edit control from the Load preset window.
Load and Delete buttons stay disabled until a preset is picked.

@Flamefire Flamefire left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure about removing the delete button from saving. I can imagine a use case where a user wants to store one but delete an existing one first to avoid confusing it with another one.

How about another layout:

<list>
<delete button>
<space>
<edit>
<save button>

So "delete" only works on the selected item

default: break;
}
}
if(ShouldBeClosed())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is this for in the ctor? How can that be true?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It could only be true in a single situation: when the ctor couldn't ensure the presets folder exists. In that case the base ctor showed an error and called Close() without building the table, so this skipped building the rest of a window that was about to close anyway.

That was a bit clunky, so I rewrote it: there's now iwAddonPresetsBase::EnsurePresetsFolder(), which iwAddons calls before opening either preset window. If the folder can't be created the user gets the error and the window is never constructed at all, so the ctor can just assume the folder is there and both guards are gone. Much more readable.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

so this skipped building the rest of a window that was about to close anyway.

IMO that solution had a big advantage: No non-local assumptions. What happens now when the folder does not exist when the window is opened?

It looks like the guards are not even required: Yes it will do some useless work, but that is never noticeable, is it? Or would it crash?

Comment thread libs/s25main/ingameWindows/iwAddonPresets.cpp Outdated
Comment thread libs/s25main/ingameWindows/iwAddonPresets.cpp Outdated
Avoids constructing a preset window just to have it close itself.
@MichalLabuda

Copy link
Copy Markdown
Contributor Author

On the Delete button: in the Save Addon Preset window it had two possible targets - the row selected in the list and the name typed in the edit box - and those differ as soon as the user types after selecting. In the Load Addon Preset window the selection is the only target for both buttons. Remoing Delete button from save window, is the only solution I could think of that makes the UI most intuitive and eliminates edit field vs. selected preset confusion (saving as whatever is in the edit field (minus leading spaces) is usually expected, not what's selected. Quite the opposite is true for delete action). This also makes the code a bit more readable and predictable.

As for your layout, it could resolve the ambiguity somewhat but then load window would need buttons rearranged too as to keep both windows visually consistent (same Delete button placement). Also it doesn't solve problems like inconsistency when you add spaces at the beginning of the existing preset name in the edit box. Before this PR, if you select a preset (it fills edit field) and then add spaces at the beginning of this existing preset name, Save will ask if you want to overwrite, because spaces at the beginning are trimmed on save and it points back to the preset you selected. At the same time clicking Delete will say "Preset not found" because it looks for exact match (with spaces).
Yet another solution to this problem could be selecting/deselecting a preset on the list depending on the edit field content. Matching preset would be selected on the list and we could disable Delete button if nothing is selected. But auto-select will face the same problem with trimming spaces. To do it right some additional real-time input processing would have to be added or auto-selection abandoned. In general, what this PR introduces is the simplest solution that mostly addresses all the problems except maybe for manual OS-side filename manipulations but there has to be limit of responsibility somewhere...

I wanted to fix it before merging the original addon presets PR but was late with pushing my work. I had it done but not fully verified/thought through yet so I lingered. That's why this follow-up came in such a short time after original PR was merged - work was already done.

@Flamefire

Copy link
Copy Markdown
Member

Yet another solution to this problem could be selecting/deselecting a preset on the list depending on the edit field content. Matching preset would be selected on the list and we could disable Delete button if nothing is selected. But auto-select will face the same problem with trimming spaces

So the only issue is that the save-window has an edit which may confuse users. Your idea there can actually be used: Msg_EditChange is called when the edit is modified. In this case the delete button can simply be disabled. It is only ever enabled when selecting a preset which fills the edit and enables the button. If the user modifies the edit it is disabled even if that happens to match an entry.
I'd say deleting a preset by typing its name and then clicking a button is unusual enough to not need to support it. But deleting a file before saving a new one seems more usual to me. And disabling the delete button when nothing is selected seems consistent too
And it seems easy enough to implement.

Or what do you think?

As for placement of controls: save window only needs to add edit and save button to the bottom. Could even be done semi-automatically by having a constant for the additional height and then using getIwHeight to get the intended position for the edit.
So the delete button and logic can be in the base

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.

2 participants