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

Error message (possibly invalid WML) while updating Add-ons #3059

Closed
Konrad22 opened this issue May 9, 2018 · 12 comments
Closed

Error message (possibly invalid WML) while updating Add-ons #3059

Konrad22 opened this issue May 9, 2018 · 12 comments
Labels
Blocker: New Stable Issues that must be resolved prior to the next stable series being released. Bug Issues involving unexpected behavior. Good first issue Issues deemed adequate for contributors without prior experience to work on. UI User interface issues, including both back-end and front-end issues.

Comments

@Konrad22
Copy link
Contributor

Konrad22 commented May 9, 2018

While updating my Add-ons, this happened:
wml
Wesnoth did not crash afterwards, I was just returned to the main screen.
I had an Add-on description open, when I used the 'Update all' button. (It's reproducible this way.)
And it apparently finished updating.

@sevu sevu added the Bug Issues involving unexpected behavior. label May 9, 2018
@CelticMinstrel
Copy link
Member

CelticMinstrel commented May 9, 2018

This most likely is a problem on the C++ side rather than invalid WML; the code should be treating the referenced widget as optional rather than mandatory (or otherwise make sure it's actually in scope before referencing it).

It would only occur in smaller windows where the description replaces the list (on a large enough resolution they're shown side-by-side).

@Konrad22
Copy link
Contributor Author

Konrad22 commented May 9, 2018

(My case is the small(er) window.)

@CelticMinstrel
Copy link
Member

I was noting window size as a factor for other people's benefit; it was clear to me that that's what you had.

@CelticMinstrel CelticMinstrel added the UI User interface issues, including both back-end and front-end issues. label May 11, 2018
@jyrkive
Copy link
Member

jyrkive commented Aug 11, 2018

@Konrad22 mentioned in Discord that it also occurs if he presses a key while having addon description open.

My first guess would be that the search field attempts to do something while the addon list is hidden.

@Konrad22
Copy link
Contributor Author

Small update: Enter or Esc do not trigger this. But things like alt gr + q for special symbols do trigger it.

@CelticMinstrel
Copy link
Member

Based on the error message, I think that @jyrkive is almost certainly correct in his assessment of the cause.

Possible fixes:

  • Treat it as optional; if it's found to be hidden (find_widget returns null), don't update anything, just note that it needs to be updated when it's shown later.
  • Ensure that the list is visible before working with it; then if it was originally hidden, hide it again when done.

@Konrad22
Copy link
Contributor Author

Konrad22 commented Dec 9, 2020

Why this needs to be fixed before 1.16:
This error message will appear (and close the add-on manager) every single time the user tries to select an older version of an add-on in the add-on description.
(Why selecting an older version is possible: #5038)

EDIT: You can also trigger this one by pressing almost any key while having opened an add-on description. Basically you are trying to filter while there is no option to filter.
grafik

@CelticMinstrel
Copy link
Member

Huh, is this still not fixed? I don't think it would be all that hard… might even be a good first issue for someone…

@soliton- soliton- added the Good first issue Issues deemed adequate for contributors without prior experience to work on. label Feb 15, 2021
@stevecotton
Copy link
Contributor

stevecotton commented May 26, 2021

There are multiple paths that result in the same error message. I've just posted a fix to one specific path in #5810, but I'm wondering if calling stacked_widget::set_find_in_all_layers(true) would be a more general solution.

Edit: stacked_widget::set_find_in_all_layers(true) does prevent the crash, but it allows keypresses to change the filter while the details panel is open. If that changes which add-ons are visible in the list, then the details panel will change to show the topmost one in the list.

@stevecotton stevecotton added the Blocker: New Stable Issues that must be resolved prior to the next stable series being released. label May 30, 2021
@stevecotton
Copy link
Contributor

As pointed out on IRC, this is going to be much easier to trigger in 1.16, and IMO it's something that should be a blocker.

stevecotton added a commit to stevecotton/wesnoth that referenced this issue May 30, 2021
…valid WML" message

This fixes two specific causes of wesnoth#3059, where code assumes that all parts of
the add-ons manager's UI are accessible, an assumption that fails when the
dialog uses a stacked widget to handle small window sizes.

It would be better to redesign the C++ for this dialog, but a quick workaround
for the observed issues is much easier.

* Fixes wesnoth#5810, triggered by using the version drop-down in small-screen mode.
* Adds a workaround for keyboard input changing the selected add-on on small screens.
@stevecotton
Copy link
Contributor

Two separate causes of this were fixed in #5820, I think it makes sense to close this issue (and open a new one if there's another way to cause the issue).

stevecotton added a commit to stevecotton/wesnoth that referenced this issue Feb 5, 2022
…mode

Even when small-screen mode is hiding the list, keyboard input of the up and
down arrow keys still selects the previous or next add-on in the list. On a
scale of "bug" to "feature", that seems useful and an understandable UX.
However, this caused another instance of issue wesnoth#3059, where code assumes that
all parts of the add-ons manager's UI are accessible, an assumption that fails
when the dialog uses a stacked widget to handle small window sizes.

Two instances of wesnoth#3059 were fixed in 050feb6:

* The one that didn't involve keyboard input provides the fix to the current issue.
  The new code in on_addon_select is the same as 050feb6 applied to
  on_selected_version_change, traversing the stacked widget to get the info.
* The one that did involve keyboard input needed a documentation update.
  In 050feb6, the workaround was to avoid triggering on_addon_select. Although
  the current commit fixes on_addon_select to handle that call, it would be poor
  UX to have a hidden filter box for a hidden list changing the selected add-on.
stevecotton added a commit to stevecotton/wesnoth that referenced this issue Feb 5, 2022
…mode

Even when small-screen mode is hiding the list, keyboard input of the up and
down arrow keys still selects the previous or next add-on in the list. On a
scale of "bug" to "feature", that seems useful and an understandable UX.
However, this caused another instance of issue wesnoth#3059, where code assumes that
all parts of the add-ons manager's UI are accessible, an assumption that fails
when the dialog uses a stacked widget to handle small window sizes.

Two instances of wesnoth#3059 were fixed in 050feb6. The one that didn't involve
keyboard input provides the fix to the current issue, as the new code in
on_addon_select is the same as 050feb6 applied to
on_selected_version_change, traversing the stacked widget to get the info.

The one that did involve keyboard input isn't addressed here, other than a minor
documentation update. In 050feb6, the workaround was to avoid refiltering the
list, even if the text in the filter box changed; while the current commit fixes
the on_addon_select function, altering the filter also needs the various dynamic_bitset
calculations to be updated too. In the meantime, 2643671 has changed the UI to
show the list's filter box on both the list screen and the details screen,
however changes to the filter string are still ignored.
stevecotton added a commit to stevecotton/wesnoth that referenced this issue Feb 6, 2022
…mode

Fix another instance of issue wesnoth#3059, where code assumes that all parts of the
add-ons manager's UI are accessible, an assumption that fails when the dialog
uses a stacked widget to handle small window sizes.

The up and down arrow keys select the previous or next add-on in the list,
even when the small-window layout is hiding the list. That feels like a feature
rather than a bug, as it's useful and an understandable UX; however it needs
the fix in this commit so that on_addon_select() doesn't throw an exception
and close the dialog. The new code is the same as the fix that 050feb6
applied to on_selected_version_change, traversing the stacked widget to get the
info.
stevecotton added a commit that referenced this issue Feb 8, 2022
…mode

Fix another instance of issue #3059, where code assumes that all parts of the
add-ons manager's UI are accessible, an assumption that fails when the dialog
uses a stacked widget to handle small window sizes.

The up and down arrow keys select the previous or next add-on in the list,
even when the small-window layout is hiding the list. That feels like a feature
rather than a bug, as it's useful and an understandable UX; however it needs
the fix in this commit so that on_addon_select() doesn't throw an exception
and close the dialog. The new code is the same as the fix that 050feb6
applied to on_selected_version_change, traversing the stacked widget to get the
info.
stevecotton added a commit to stevecotton/wesnoth that referenced this issue Feb 8, 2022
…mode

Fix another instance of issue wesnoth#3059, where code assumes that all parts of the
add-ons manager's UI are accessible, an assumption that fails when the dialog
uses a stacked widget to handle small window sizes.

The up and down arrow keys select the previous or next add-on in the list,
even when the small-window layout is hiding the list. That feels like a feature
rather than a bug, as it's useful and an understandable UX; however it needs
the fix in this commit so that on_addon_select() doesn't throw an exception
and close the dialog. The new code is the same as the fix that 050feb6
applied to on_selected_version_change, traversing the stacked widget to get the
info.

(cherry picked from commit 6a72b2e)
@stevecotton
Copy link
Contributor

While testing #6499, I found another instance: clicking "Update All" in the details view successfully downloads and installs all updates, but then returns to the title screen with this "possibly invalid WML" error message. As the add-ons do get successfully updated first, I think that's trivial enough that I'm not logging a separate bug for it.

stevecotton added a commit that referenced this issue Feb 8, 2022
…mode

Fix another instance of issue #3059, where code assumes that all parts of the
add-ons manager's UI are accessible, an assumption that fails when the dialog
uses a stacked widget to handle small window sizes.

The up and down arrow keys select the previous or next add-on in the list,
even when the small-window layout is hiding the list. That feels like a feature
rather than a bug, as it's useful and an understandable UX; however it needs
the fix in this commit so that on_addon_select() doesn't throw an exception
and close the dialog. The new code is the same as the fix that 050feb6
applied to on_selected_version_change, traversing the stacked widget to get the
info.

(cherry picked from commit 6a72b2e)
Asheviere pushed a commit to Asheviere/wesnoth that referenced this issue Feb 18, 2022
…mode

Fix another instance of issue wesnoth#3059, where code assumes that all parts of the
add-ons manager's UI are accessible, an assumption that fails when the dialog
uses a stacked widget to handle small window sizes.

The up and down arrow keys select the previous or next add-on in the list,
even when the small-window layout is hiding the list. That feels like a feature
rather than a bug, as it's useful and an understandable UX; however it needs
the fix in this commit so that on_addon_select() doesn't throw an exception
and close the dialog. The new code is the same as the fix that 050feb6
applied to on_selected_version_change, traversing the stacked widget to get the
info.

(cherry picked from commit 6a72b2e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocker: New Stable Issues that must be resolved prior to the next stable series being released. Bug Issues involving unexpected behavior. Good first issue Issues deemed adequate for contributors without prior experience to work on. UI User interface issues, including both back-end and front-end issues.
Projects
None yet
Development

No branches or pull requests

6 participants