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 Manual ISO selection option #1934

Merged
merged 2 commits into from Jun 19, 2017

Conversation

Projects
None yet
7 participants
@ssakash
Member

ssakash commented May 8, 2017

Summary of changes:

  • Add a Manual ISO selection option which allows you to select your ISO via the file explorer each time whenever you full/fast boot. This one is for you @vsub ❤️

PCSX2 by default only allows you to hold 12 ISOs in the recent ISO list and when you try to add anymore, the previous ones will replaced by your newer ISO selection. The only way to modify the total limit of ISOs capable of being stored in the recent ISO list is to change the value of an INI variable, obviously no end user would be aware of it.

Hence for people with large amount of ISOs in their storage devices, it would be ideal if they could just directly look for their ISOs instead of looking through the recent ISO list which is not capable of storing all of their games.

Another alternative would be to increase the default maximum no.of ISOs allowed in the recent ISO list but that would just make the list a bit larger and at one point it would even grow big enough to touch your taskbar! there's no possible way that the recent ISO list would be able to accommodate all the ISOs of say someone who has a collection of over 50+ games. Also it's not desirable behavior to go to CDVD -> ISO Selector -> Browse.. to add individual ISOs each and ever single time. -_-

To avoid such issues, I'm proposing to add this option which would directly take you to the file explorer when you're doing a full/fast boot. Any opinion, guys?

@willkuer

This comment has been minimized.

Show comment
Hide comment
@willkuer

willkuer May 8, 2017

Contributor

Very good feature! I always wanted ti have something like that. However somehow I got used to the inconvinient Browse button.

Maybe 'Browse upon boot' or 'Iso selection upon boot' or something like that? Without your commit explanation I wouldn't understand what 'Manual ISO selection' means.

Also I didn't check the implementation but according to your screenshot you should probably additionally disable the 'Browse' menu entry whenever manual iso is enabled. I think it should then also be placed on top of Browse as the first element. Maybe seperated by another menu seperator as the browse button is then a slave to the checkbox.

Or you can add an 'Iso selection' menu entry with two real slave entries (only appear on mouse hover on the right) which are radio buttons with label 'Manual ISO selection' or whatever variant and 'Remember ISO selection' or 'Reload last booted game'

Also manual should be probably lower case or selection uppercase if you prefer title case style over normal case style.

Contributor

willkuer commented May 8, 2017

Very good feature! I always wanted ti have something like that. However somehow I got used to the inconvinient Browse button.

Maybe 'Browse upon boot' or 'Iso selection upon boot' or something like that? Without your commit explanation I wouldn't understand what 'Manual ISO selection' means.

Also I didn't check the implementation but according to your screenshot you should probably additionally disable the 'Browse' menu entry whenever manual iso is enabled. I think it should then also be placed on top of Browse as the first element. Maybe seperated by another menu seperator as the browse button is then a slave to the checkbox.

Or you can add an 'Iso selection' menu entry with two real slave entries (only appear on mouse hover on the right) which are radio buttons with label 'Manual ISO selection' or whatever variant and 'Remember ISO selection' or 'Reload last booted game'

Also manual should be probably lower case or selection uppercase if you prefer title case style over normal case style.

@gregory38

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 May 8, 2017

Contributor

Tbh 12 iso entries is really small. I'm also in favor to increase it. Maybe 20-25 would be better, dunno if it fit on small display.

Contributor

gregory38 commented May 8, 2017

Tbh 12 iso entries is really small. I'm also in favor to increase it. Maybe 20-25 would be better, dunno if it fit on small display.

@avih

This comment has been minimized.

Show comment
Hide comment
@avih

avih May 8, 2017

Member

I also think we could increase the maximum to 20-25. However, I haven't tested how it behaves when the list becomes taller than the screen. Though I doubt 20 will be too much on any screen.

Another thing we could do, is improve the MRU behavior (i.e. which files get to the top of the list and when). At the time when I implemented the MRU I thought that keeping the ISO's place at the list is more important than moving the last used ISO to the top, but maybe we can change that too. Should be trivial, and I think it would feel some natural to users.

Member

avih commented May 8, 2017

I also think we could increase the maximum to 20-25. However, I haven't tested how it behaves when the list becomes taller than the screen. Though I doubt 20 will be too much on any screen.

Another thing we could do, is improve the MRU behavior (i.e. which files get to the top of the list and when). At the time when I implemented the MRU I thought that keeping the ISO's place at the list is more important than moving the last used ISO to the top, but maybe we can change that too. Should be trivial, and I think it would feel some natural to users.

@ssakash

This comment has been minimized.

Show comment
Hide comment
@ssakash

ssakash May 9, 2017

Member

disable the 'Browse' menu entry whenever manual iso is enabled. I think it should then also be placed on top of Browse as the first element. Maybe seperated by another menu seperator as the browse button is then a slave to the checkbox.

Also manual should be probably lower case or selection uppercase if you prefer title case style over normal case style.

Done, thanks a lot for the suggestions. 👍

Maybe 'Browse upon boot' or 'Iso selection upon boot' or something like that? Without your commit explanation I wouldn't understand what 'Manual ISO selection' means.

There's a help text which will be displayed on the console log window when the user hovers over the menu item, maybe that might help? Though to be honest, I think the text Manual ISO Selection in itself is rather easy to understand for users. (Hopefully I'm not the only one who feels so?)

Tbh 12 iso entries is really small. I'm also in favor to increase it. Maybe 20-25 would be better, dunno if it fit on small display.

25 and the list is reaching my taskbar. (I've added a commit to temporarily raise it to 15, personal reasons really :P)

Member

ssakash commented May 9, 2017

disable the 'Browse' menu entry whenever manual iso is enabled. I think it should then also be placed on top of Browse as the first element. Maybe seperated by another menu seperator as the browse button is then a slave to the checkbox.

Also manual should be probably lower case or selection uppercase if you prefer title case style over normal case style.

Done, thanks a lot for the suggestions. 👍

Maybe 'Browse upon boot' or 'Iso selection upon boot' or something like that? Without your commit explanation I wouldn't understand what 'Manual ISO selection' means.

There's a help text which will be displayed on the console log window when the user hovers over the menu item, maybe that might help? Though to be honest, I think the text Manual ISO Selection in itself is rather easy to understand for users. (Hopefully I'm not the only one who feels so?)

Tbh 12 iso entries is really small. I'm also in favor to increase it. Maybe 20-25 would be better, dunno if it fit on small display.

25 and the list is reaching my taskbar. (I've added a commit to temporarily raise it to 15, personal reasons really :P)

@gregory38

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 May 9, 2017

Contributor

Why not 20 ? What is your screen resolution ?

Contributor

gregory38 commented May 9, 2017

Why not 20 ? What is your screen resolution ?

@ssakash

This comment has been minimized.

Show comment
Hide comment
@ssakash

ssakash May 9, 2017

Member

Why not 20 ?

I'm not sure about people with even lower screen size so I left it at 15 just to be on the safe side. I don't mind updating it to 20, it's just troublesome when the list gets larger to a point where it takes extra effort to select the bottom most entry.

What is your screen resolution ?

1366 x 768 (It's the most common resolution for 15.6” laptops)

Member

ssakash commented May 9, 2017

Why not 20 ?

I'm not sure about people with even lower screen size so I left it at 15 just to be on the safe side. I don't mind updating it to 20, it's just troublesome when the list gets larger to a point where it takes extra effort to select the bottom most entry.

What is your screen resolution ?

1366 x 768 (It's the most common resolution for 15.6” laptops)

@gregory38

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 May 9, 2017

Contributor

So 20 should fit on all displays (or at least 18). I don't understand why manufacturers don't upgrade display to full-hd to boost sell.

Contributor

gregory38 commented May 9, 2017

So 20 should fit on all displays (or at least 18). I don't understand why manufacturers don't upgrade display to full-hd to boost sell.

@avih

This comment has been minimized.

Show comment
Hide comment
@avih

avih May 9, 2017

Member

I think just changing the default list size is enough (I'd choose 20 but I'm fine with other values too).

I don't really like the original/rest of this PR though.

Before:

  • To boot a different game: choose one from the MRU (or open the ISO browser from the same menu) and then boot.
  • To boot the same game: just boot.

After (assuming the user has the new option enabled):

  • To boot a different game: Boot, then find your ISO in the browser.
  • To boot the same game: same as booting a different game.

So the only scenario where it saves clicks (two click) is if you're changing games all the time and your list is too short to hold the all the games you're switching between.

To me, it's not worth "9 files modified" of this PR, as small as the changes might be.

Member

avih commented May 9, 2017

I think just changing the default list size is enough (I'd choose 20 but I'm fine with other values too).

I don't really like the original/rest of this PR though.

Before:

  • To boot a different game: choose one from the MRU (or open the ISO browser from the same menu) and then boot.
  • To boot the same game: just boot.

After (assuming the user has the new option enabled):

  • To boot a different game: Boot, then find your ISO in the browser.
  • To boot the same game: same as booting a different game.

So the only scenario where it saves clicks (two click) is if you're changing games all the time and your list is too short to hold the all the games you're switching between.

To me, it's not worth "9 files modified" of this PR, as small as the changes might be.

@willkuer

This comment has been minimized.

Show comment
Hide comment
@willkuer

willkuer May 9, 2017

Contributor

@avih I would like to have this feature. Seems like vsub as well. The nice thing about this PR is that you can always have the old behavior if you like it more.

@ssakash Adding entries to the list might also be related to the DPI settings. Possibly 20 is fine on default settings but not if the DPI are enhanced.

Contributor

willkuer commented May 9, 2017

@avih I would like to have this feature. Seems like vsub as well. The nice thing about this PR is that you can always have the old behavior if you like it more.

@ssakash Adding entries to the list might also be related to the DPI settings. Possibly 20 is fine on default settings but not if the DPI are enhanced.

PCSX2-WX: Increment max ISO count in recent ISO list
The default 12 is rather low and won't suffice for most cases, updating
it to 20 to give some extra space for additional ISOs. Incrementing it
to an even higher value might not be so good as it consumes lots of
vertical space, not a nice idea for people with smaller screens.
@Hirato

This comment has been minimized.

Show comment
Hide comment
@Hirato

Hirato May 9, 2017

Usually when Menus get too big to fix on screen, they either get scrolling arrows at the top and bottom, or they overflow into a second column (or third, or fourth).

Hirato commented May 9, 2017

Usually when Menus get too big to fix on screen, they either get scrolling arrows at the top and bottom, or they overflow into a second column (or third, or fourth).

@turtleli

This comment has been minimized.

Show comment
Hide comment
@turtleli

turtleli May 10, 2017

Member

There's a help text which will be displayed on the console log window when the user hovers over the menu item, maybe that might help? Though to be honest, I think the text Manual ISO Selection in itself is rather easy to understand for users. (Hopefully I'm not the only one who feels so?)

I find it hard to understand. ;) It's probably best if a user can understand what a menu item does without having to look at the help text. I dunno what would be better though (maybe "Always ask when booting" or one of willkuer's suggestions or some other random thought).

I'm not sure it's a great idea to disable the browse button (at least after booting) when the feature is enabled. What if the user has to swap discs?

Member

turtleli commented May 10, 2017

There's a help text which will be displayed on the console log window when the user hovers over the menu item, maybe that might help? Though to be honest, I think the text Manual ISO Selection in itself is rather easy to understand for users. (Hopefully I'm not the only one who feels so?)

I find it hard to understand. ;) It's probably best if a user can understand what a menu item does without having to look at the help text. I dunno what would be better though (maybe "Always ask when booting" or one of willkuer's suggestions or some other random thought).

I'm not sure it's a great idea to disable the browse button (at least after booting) when the feature is enabled. What if the user has to swap discs?

@gregory38

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 May 11, 2017

Contributor

It isn't expected that menu got out of screen.
http://wxwidgets.10942.n7.nabble.com/Getting-wxMenu-size-td88159.html

Contributor

gregory38 commented May 11, 2017

It isn't expected that menu got out of screen.
http://wxwidgets.10942.n7.nabble.com/Getting-wxMenu-size-td88159.html

@ssakash

This comment has been minimized.

Show comment
Hide comment
@ssakash

ssakash May 19, 2017

Member

I find it hard to understand. ;) It's probably best if a user can understand what a menu item does without having to look at the help text. I dunno what would be better though (maybe "Always ask when booting" or one of willkuer's suggestions or some other random thought).

Fair enough, changed it to Always ask when booting just to be on the safe side. I figured you might nitpick if I chose a name of my own. :P

Member

ssakash commented May 19, 2017

I find it hard to understand. ;) It's probably best if a user can understand what a menu item does without having to look at the help text. I dunno what would be better though (maybe "Always ask when booting" or one of willkuer's suggestions or some other random thought).

Fair enough, changed it to Always ask when booting just to be on the safe side. I figured you might nitpick if I chose a name of my own. :P

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

This comment has been minimized.

Show comment
Hide comment
@ssakash

ssakash Jun 9, 2017

Member

@turtleli

Should this be merged before 1.6 (or) postponed after 1.6 release? I'll let you handle the decision, :)

Member

ssakash commented Jun 9, 2017

@turtleli

Should this be merged before 1.6 (or) postponed after 1.6 release? I'll let you handle the decision, :)

@turtleli

This comment has been minimized.

Show comment
Hide comment
@turtleli

turtleli Jun 12, 2017

Member

Well, I'm not sure about the browse button behaviour when the option is enabled. I think the behaviour should be:

  • Game/BIOS not booted/shutdown -> Browse button disabled
  • Otherwise -> Browse button enabled
Member

turtleli commented Jun 12, 2017

Well, I'm not sure about the browse button behaviour when the option is enabled. I think the behaviour should be:

  • Game/BIOS not booted/shutdown -> Browse button disabled
  • Otherwise -> Browse button enabled
@ssakash

This comment has been minimized.

Show comment
Hide comment
@ssakash

ssakash Jun 12, 2017

Member

Well, I'm not sure about the browse button behaviour when the option is enabled. I think the behaviour should be:

Added the behavior which you have described, anything else left to do?

Member

ssakash commented Jun 12, 2017

Well, I'm not sure about the browse button behaviour when the option is enabled. I think the behaviour should be:

Added the behavior which you have described, anything else left to do?

@turtleli

This comment has been minimized.

Show comment
Hide comment
@turtleli

turtleli Jun 13, 2017

Member

The browse button stuff doesn't work properly if I boot an ISO from the command line - the Browse button remains disabled.

I also wonder whether the browse button behaviour should be extended to the recent ISO list too (I think it's a good idea, but I'm unsure).

Member

turtleli commented Jun 13, 2017

The browse button stuff doesn't work properly if I boot an ISO from the command line - the Browse button remains disabled.

I also wonder whether the browse button behaviour should be extended to the recent ISO list too (I think it's a good idea, but I'm unsure).

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

This comment has been minimized.

Show comment
Hide comment
@ssakash

ssakash Jun 15, 2017

Member

The browse button stuff doesn't work properly if I boot an ISO from the command line - the Browse button remains disabled.

Nice catch, I pushed a more generic solution to handle it.

I also wonder whether the browse button behaviour should be extended to the recent ISO list too (I think it's a good idea, but I'm unsure).

I also think it's a good idea, implemented it.

Member

ssakash commented Jun 15, 2017

The browse button stuff doesn't work properly if I boot an ISO from the command line - the Browse button remains disabled.

Nice catch, I pushed a more generic solution to handle it.

I also wonder whether the browse button behaviour should be extended to the recent ISO list too (I think it's a good idea, but I'm unsure).

I also think it's a good idea, implemented it.

@turtleli

This comment has been minimized.

Show comment
Hide comment
@turtleli

turtleli Jun 17, 2017

Member

Something isn't right since the latest update - it potentially crashes when closing PCSX2.

Member

turtleli commented Jun 17, 2017

Something isn't right since the latest update - it potentially crashes when closing PCSX2.

@ssakash

This comment has been minimized.

Show comment
Hide comment
@ssakash

ssakash Jun 18, 2017

Member

Something isn't right since the latest update - it potentially crashes when closing PCSX2.

Should be sorted out now I guess.

Member

ssakash commented Jun 18, 2017

Something isn't right since the latest update - it potentially crashes when closing PCSX2.

Should be sorted out now I guess.

Show outdated Hide outdated pcsx2/gui/RecentIsoList.cpp Outdated
PCSX2-WX: Add "Always ask when booting" option
When enabled, this option opens the file explorer to directly select the
ISO at each boot instances instead of relying on the Recent ISO list.

@turtleli turtleli merged commit 58102a3 into PCSX2:master Jun 19, 2017

2 checks passed

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

@ssakash ssakash deleted the ssakash:manualISO branch Jun 20, 2017

@vsub

This comment has been minimized.

Show comment
Hide comment
@vsub

vsub Jul 8, 2017

Not that I can try this feature right now but thanks.

vsub commented Jul 8, 2017

Not that I can try this feature right now but thanks.

@vsub

This comment has been minimized.

Show comment
Hide comment
@vsub

vsub Apr 3, 2018

@ssakash
Sorry for the late responce

Seems to be working fine but there is one small problem(not really a big deal(I think)because I can just restart pcsx2 after that)

If the option is enabled and the first things you use is anything other that ISO(for exaple No Disk to access the bios),when you switch to ISO,you are asked what to do,swap the disk or reset.

Both options lead to the "CDVD plugin failed to open" message and then it ask you if you want to check the plugins or not.

With "Swap Disk" you can just hit cancel and then fast\full boot to get asked what game to load
With "Reset",you will again get the same question but there is nothing you can do there because pcsx2 freezes and you have to kill the process

vsub commented Apr 3, 2018

@ssakash
Sorry for the late responce

Seems to be working fine but there is one small problem(not really a big deal(I think)because I can just restart pcsx2 after that)

If the option is enabled and the first things you use is anything other that ISO(for exaple No Disk to access the bios),when you switch to ISO,you are asked what to do,swap the disk or reset.

Both options lead to the "CDVD plugin failed to open" message and then it ask you if you want to check the plugins or not.

With "Swap Disk" you can just hit cancel and then fast\full boot to get asked what game to load
With "Reset",you will again get the same question but there is nothing you can do there because pcsx2 freezes and you have to kill the process

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment