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

Game Load: Show list of enabled modification. #3495

Merged
merged 4 commits into from Aug 27, 2018

Conversation

Projects
None yet
4 participants
@jostephd
Member

jostephd commented Aug 24, 2018

screenshot_2018-08-24_18-33-20

This PR adds the "Modifications:" header and bulleted list. It shows the mod name if the mod is installed, else mod id, as done for campaigns.

Similar to #3478.

@Pentarctagon

This comment has been minimized.

Show comment
Hide comment
@Pentarctagon

Pentarctagon Aug 24, 2018

Member

What would happen if the user had 20, 30, or more mods enabled, for whatever reason and it became too long to display?

Member

Pentarctagon commented Aug 24, 2018

What would happen if the user had 20, 30, or more mods enabled, for whatever reason and it became too long to display?

@jostephd

This comment has been minimized.

Show comment
Hide comment
@jostephd

jostephd Aug 24, 2018

Member

The dialog would have a vertical scrollbar next to the file list scrollbar. Not pretty but it works.

screenshot_2018-08-24_21-56-13
screenshot_2018-08-24_21-56-49

Member

jostephd commented Aug 24, 2018

The dialog would have a vertical scrollbar next to the file list scrollbar. Not pretty but it works.

screenshot_2018-08-24_21-56-13
screenshot_2018-08-24_21-56-49

@jostephd

This comment has been minimized.

Show comment
Hide comment
@jostephd

jostephd Aug 24, 2018

Member

There's a bug: the vertical space allocated to the mods list is determined by the first savegame. If the second savegame has more mods than the first savegames, the excess ones won't be shown.

Member

jostephd commented Aug 24, 2018

There's a bug: the vertical space allocated to the mods list is determined by the first savegame. If the second savegame has more mods than the first savegames, the excess ones won't be shown.

@gfgtdf

This comment has been minimized.

Show comment
Hide comment
@gfgtdf

gfgtdf Aug 24, 2018

Contributor

The dialog would have a vertical scrollbar next to the file list scrollbar. Not pretty but it works.

I think It'd be better if there would appear a scrollbar in the left-side-panel in that case, and of course also only for those savegames that actuall have too many mods enabled.

Contributor

gfgtdf commented Aug 24, 2018

The dialog would have a vertical scrollbar next to the file list scrollbar. Not pretty but it works.

I think It'd be better if there would appear a scrollbar in the left-side-panel in that case, and of course also only for those savegames that actuall have too many mods enabled.

@jostephd

This comment has been minimized.

Show comment
Hide comment
@jostephd

jostephd Aug 25, 2018

Member

Done -
screenshot_2018-08-25_16-04-50

Member

jostephd commented Aug 25, 2018

Done -
screenshot_2018-08-25_16-04-50

@gfgtdf

This comment has been minimized.

Show comment
Hide comment
@gfgtdf

gfgtdf Aug 25, 2018

Contributor

Done -

i assume this also fixed the bug you meantione eariler ?

Contributor

gfgtdf commented Aug 25, 2018

Done -

i assume this also fixed the bug you meantione eariler ?

@jostephd

This comment has been minimized.

Show comment
Hide comment
@jostephd

jostephd Aug 25, 2018

Member

Yes, that bug was fixed by 57afe4d. Sorry for being unclear.

Member

jostephd commented Aug 25, 2018

Yes, that bug was fixed by 57afe4d. Sorry for being unclear.

@gfgtdf

This comment has been minimized.

Show comment
Hide comment
@gfgtdf

gfgtdf Aug 25, 2018

Contributor

does that mean the dialogs size can now change when selecting another game? Usually window.invalidate_layout shodul be avoided becasue it causes things like that.

Contributor

gfgtdf commented Aug 25, 2018

does that mean the dialogs size can now change when selecting another game? Usually window.invalidate_layout shodul be avoided becasue it causes things like that.

@jostephd

This comment has been minimized.

Show comment
Hide comment
@jostephd

jostephd Aug 25, 2018

Member

The dialog's size does not change when selecting another game. The only thing that changes is the location of the leader list, but it remains visible at all times. If the invalidate_layout were removed, a vertical scrollbar would be used even when a re-layout with no vertical scrollbar was possible (you can see this whenever selecting a savegame that has more mods than the newest savegame).

Member

jostephd commented Aug 25, 2018

The dialog's size does not change when selecting another game. The only thing that changes is the location of the leader list, but it remains visible at all times. If the invalidate_layout were removed, a vertical scrollbar would be used even when a re-layout with no vertical scrollbar was possible (you can see this whenever selecting a savegame that has more mods than the newest savegame).

@jyrkive

@gfgtdf window.invalidate_layout() should not be avoided. It's better to invalidate the layout more often than necessary than to skip invalidation when we actually need it.

Show outdated Hide outdated src/gui/dialogs/game_load.cpp Outdated
Show outdated Hide outdated src/gui/dialogs/game_load.cpp Outdated
Show outdated Hide outdated src/gui/dialogs/game_load.cpp Outdated

jostephd added some commits Aug 24, 2018

Game Load: Show list of enabled modification.
Show the mod name if the mod is installed, else mod id, as done for campaigns.
Game Load: Don't reserve space for the the horizontal scrollbar when …
…it's not needed.

I think it looks better this way.
@gfgtdf

This comment has been minimized.

Show comment
Hide comment
@gfgtdf

gfgtdf Aug 27, 2018

Contributor

no, invalidate_layout must be avoided at least in the current gui2 implementation, in the worst case it causes widgets to 'jump around' which makes a really crappy impression to the user, also it can be quite slow if the dialogue is complicated ( at least that's the state from when I checked this stuff last time.) , If you 'need' a relayout then the is prevails something else wrong or you are using gui2 in a way that it was not designed to.

Ideally a (usable) relayout would only layout the content on the highest fixes-size widgets that the 'problematic' widgets is in. or rather, we should add a 'layout blocker' widget that makes layout calls only relayout it's content. when relayout is calls inside.

Contributor

gfgtdf commented Aug 27, 2018

no, invalidate_layout must be avoided at least in the current gui2 implementation, in the worst case it causes widgets to 'jump around' which makes a really crappy impression to the user, also it can be quite slow if the dialogue is complicated ( at least that's the state from when I checked this stuff last time.) , If you 'need' a relayout then the is prevails something else wrong or you are using gui2 in a way that it was not designed to.

Ideally a (usable) relayout would only layout the content on the highest fixes-size widgets that the 'problematic' widgets is in. or rather, we should add a 'layout blocker' widget that makes layout calls only relayout it's content. when relayout is calls inside.

@jyrkive

This comment has been minimized.

Show comment
Hide comment
@jyrkive

jyrkive Aug 27, 2018

Member

A relayout is always needed when any GUI2 widget grows, that's how GUI2 is designed.

In some cases you can get away by firing a REQUEST_PLACEMENT event instead (but even it will fall back to full relayout if necessary).

Member

jyrkive commented Aug 27, 2018

A relayout is always needed when any GUI2 widget grows, that's how GUI2 is designed.

In some cases you can get away by firing a REQUEST_PLACEMENT event instead (but even it will fall back to full relayout if necessary).

@jyrkive jyrkive merged commit 14abecb into wesnoth:1.14 Aug 27, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jyrkive added a commit that referenced this pull request Aug 27, 2018

jostephd added a commit to jostephd/wesnoth that referenced this pull request Sep 16, 2018

jostephd added a commit to jostephd/wesnoth that referenced this pull request Oct 6, 2018

jostephd added a commit to jostephd/wesnoth that referenced this pull request Oct 7, 2018

@jostephd jostephd referenced this pull request Oct 7, 2018

Closed

Working master branch #3603

@jostephd jostephd referenced this pull request Oct 14, 2018

Open

Forward-port 1.14 to development #3614

637 of 958 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment