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

PCSX2-WX: Add clear list option to ISO Selector #2080

Merged
merged 1 commit into from Oct 10, 2017

Conversation

Projects
None yet
7 participants
@RedPanda4552
Contributor

RedPanda4552 commented Sep 28, 2017

Adds "Clear ISO List" option to ISO Selector (Issue #2071).

A look at the UI before clearing, with all my iso and bin files: https://i.imgur.com/YnPCAXQ.png
I was kind enough to give users an opportunity to change their mind: https://i.imgur.com/VUVo5l0.png
And after clearning: https://i.imgur.com/Wvsdzfn.png

Closes #2071

@lightningterror

This comment has been minimized.

Show comment
Hide comment
@lightningterror

lightningterror Sep 28, 2017

Member

Looks like you beat @ssakash to it 😆

Tested. Works properly.

Member

lightningterror commented Sep 28, 2017

Looks like you beat @ssakash to it 😆

Tested. Works properly.

@ssakash

This comment has been minimized.

Show comment
Hide comment
@ssakash

ssakash Sep 28, 2017

Member

I'm wondering if it might be a good idea to remove/gray out the menu item when there are no ISOs present in the list. Any opinion? (Personally I'd prefer destroying and appending the item over graying it out using Enable())

Turtleli was a bit faster than me on adding that Changes translatable strings label. :/

Member

ssakash commented Sep 28, 2017

I'm wondering if it might be a good idea to remove/gray out the menu item when there are no ISOs present in the list. Any opinion? (Personally I'd prefer destroying and appending the item over graying it out using Enable())

Turtleli was a bit faster than me on adding that Changes translatable strings label. :/

@lightningterror

This comment has been minimized.

Show comment
Hide comment
@lightningterror

lightningterror Sep 28, 2017

Member

I like your suggestion. If there are no isos then the button should disappear. Having it just grey out will make the list bigger for no reason when it's not needed.

Member

lightningterror commented Sep 28, 2017

I like your suggestion. If there are no isos then the button should disappear. Having it just grey out will make the list bigger for no reason when it's not needed.

@avih

This comment has been minimized.

Show comment
Hide comment
@avih

avih Sep 28, 2017

Member

The original intent with the grayed out items was that you could put your ISOs on an external drive and plug it in or out, and you'll know when they're available. They should get out of the list anyway when you open new ISOs. But feel free to remove it.

Member

avih commented Sep 28, 2017

The original intent with the grayed out items was that you could put your ISOs on an external drive and plug it in or out, and you'll know when they're available. They should get out of the list anyway when you open new ISOs. But feel free to remove it.

@pgert

This comment has been minimized.

Show comment
Hide comment
@pgert

pgert Sep 28, 2017

Contributor

Please have the Separator lines between "Always ask when booting",
"Browse..." and "Clear ISO list" removed.
The one between these optons and the ISO-list is enough.

Contributor

pgert commented Sep 28, 2017

Please have the Separator lines between "Always ask when booting",
"Browse..." and "Clear ISO list" removed.
The one between these optons and the ISO-list is enough.

@RedPanda4552

This comment has been minimized.

Show comment
Hide comment
@RedPanda4552

RedPanda4552 Sep 28, 2017

Contributor

Latest commit makes the list item and separator generate in time with the ISO list itself instead of the entire sub menu.

One thing I would like to draw attention to is that doing so required MainFrame.h to be included in RecentIsoList.cpp, for the MenuIdentifiers enum to be visible. It feels dirty to include an entire header file for one enum. Is there a better way to go about this or is this an acceptable practice?

Contributor

RedPanda4552 commented Sep 28, 2017

Latest commit makes the list item and separator generate in time with the ISO list itself instead of the entire sub menu.

One thing I would like to draw attention to is that doing so required MainFrame.h to be included in RecentIsoList.cpp, for the MenuIdentifiers enum to be visible. It feels dirty to include an entire header file for one enum. Is there a better way to go about this or is this an acceptable practice?

@MrCK1

This comment has been minimized.

Show comment
Hide comment
@MrCK1

MrCK1 Sep 28, 2017

Member

@RedPanda4552 The recent ISO list is considered part of the mainframe. That's what all the Wx menu systems are based off of. Basically, it's necessary for compilation.

Member

MrCK1 commented Sep 28, 2017

@RedPanda4552 The recent ISO list is considered part of the mainframe. That's what all the Wx menu systems are based off of. Basically, it's necessary for compilation.

@RedPanda4552

This comment has been minimized.

Show comment
Hide comment
@RedPanda4552

RedPanda4552 Sep 28, 2017

Contributor

That helps but also raises another question. Is there a known reason RecentIsoList.cpp didn't include MainFrame.h before these changes?

Contributor

RedPanda4552 commented Sep 28, 2017

That helps but also raises another question. Is there a known reason RecentIsoList.cpp didn't include MainFrame.h before these changes?

@turtleli

This comment has been minimized.

Show comment
Hide comment
@turtleli

turtleli Sep 29, 2017

Member

I think you need to include App.h instead of MainFrame.h since that's where the enum actually is.

Anyway, the current behaviour isn't that nice if you clear the ISO list while a game is running. Test case:

  1. Boot game.
  2. Clear ISO list.
  3. Pause.
  4. Resume -> Cannot resume.
Member

turtleli commented Sep 29, 2017

I think you need to include App.h instead of MainFrame.h since that's where the enum actually is.

Anyway, the current behaviour isn't that nice if you clear the ISO list while a game is running. Test case:

  1. Boot game.
  2. Clear ISO list.
  3. Pause.
  4. Resume -> Cannot resume.

@lightningterror lightningterror changed the title from Add clear list option to ISO Selector to PCSX2-WX: Add clear list option to ISO Selector Sep 29, 2017

@RedPanda4552

This comment has been minimized.

Show comment
Hide comment
@RedPanda4552

RedPanda4552 Sep 29, 2017

Contributor

Latest commit should resolve. The CurrentIso field in the ini was being wiped regardless of running state, which when the CDVD plugin is pausing and resuming, would understandably cause problems.

Contributor

RedPanda4552 commented Sep 29, 2017

Latest commit should resolve. The CurrentIso field in the ini was being wiped regardless of running state, which when the CDVD plugin is pausing and resuming, would understandably cause problems.

@lightningterror

This comment has been minimized.

Show comment
Hide comment
@lightningterror

lightningterror Sep 30, 2017

Member

I think you can squash it if it's ready for merge.

Member

lightningterror commented Sep 30, 2017

I think you can squash it if it's ready for merge.

@RedPanda4552

This comment has been minimized.

Show comment
Hide comment
@RedPanda4552

RedPanda4552 Sep 30, 2017

Contributor

And squashed.

Contributor

RedPanda4552 commented Sep 30, 2017

And squashed.

Show outdated Hide outdated pcsx2/gui/MainMenuClicks.cpp Outdated
Show outdated Hide outdated pcsx2/gui/RecentIsoList.cpp Outdated
Show outdated Hide outdated pcsx2/gui/RecentIsoList.cpp Outdated
Show outdated Hide outdated pcsx2/gui/MainMenuClicks.cpp Outdated
Show outdated Hide outdated pcsx2/gui/MainMenuClicks.cpp Outdated
Show outdated Hide outdated pcsx2/gui/MainMenuClicks.cpp Outdated
@RedPanda4552

This comment has been minimized.

Show comment
Hide comment
@RedPanda4552

RedPanda4552 Oct 2, 2017

Contributor

Review changes made and squashed.

Contributor

RedPanda4552 commented Oct 2, 2017

Review changes made and squashed.

Show outdated Hide outdated pcsx2/gui/RecentIsoList.cpp Outdated
@turtleli

This comment has been minimized.

Show comment
Hide comment
@turtleli

turtleli Oct 3, 2017

Member

Code seems fine to me.

The buttons being Yes+Cancel feels slightly odd (I generally expect Yes+No or Ok+Cancel) but maybe that's just me.

Member

turtleli commented Oct 3, 2017

Code seems fine to me.

The buttons being Yes+Cancel feels slightly odd (I generally expect Yes+No or Ok+Cancel) but maybe that's just me.

Show outdated Hide outdated pcsx2/gui/MainMenuClicks.cpp Outdated

@ssakash ssakash added the Needs work label Oct 7, 2017

@turtleli turtleli removed the Needs work label Oct 9, 2017

@turtleli turtleli merged commit 7a78441 into PCSX2:master Oct 10, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment