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

Filter non-executable files out of the game list. #4922

Merged
merged 1 commit into from Sep 17, 2019
Merged

Filter non-executable files out of the game list. #4922

merged 1 commit into from Sep 17, 2019

Conversation

Steveice10
Copy link
Contributor

@Steveice10 Steveice10 commented Sep 7, 2019

Currently, installing game CIAs will add a bunch of unusable entries for extra NCCH files like manual data. This change checks the executable bit in the NCCH header and filters non-executable entries out of the game list.

Before:
before

After:
after


This change is Reviewable

* @param out_executable Reference to store the executable flag into.
* @return ResultStatus result of function
*/
virtual ResultStatus IsExecutable(u8& out_executable) {
Copy link
Member

@lioncash lioncash Sep 7, 2019

Choose a reason for hiding this comment

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

This should likely be a const member function, since a query shouldn't modify the loader's internal state.

Copy link
Contributor Author

@Steveice10 Steveice10 Sep 7, 2019

Choose a reason for hiding this comment

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

The function needs to be able to load the backing NCCH if it isn't already, so it can read the header data.

Copy link
Member

@zhaowenlan1779 zhaowenlan1779 Sep 8, 2019

Choose a reason for hiding this comment

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

This should likely be a bool

@@ -198,6 +198,15 @@ ResultStatus AppLoader_NCCH::Load(std::shared_ptr<Kernel::Process>& process) {
return ResultStatus::Success;
}

ResultStatus AppLoader_NCCH::IsExecutable(u8& out_executable) {
Loader::ResultStatus result = overlay_ncch->Load();
Copy link
Member

@lioncash lioncash Sep 7, 2019

Choose a reason for hiding this comment

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

This should probably not be loading the entire NCCH just to query whether or not it's executable. It's kind of overkill in this case.

Copy link
Member

@wwylele wwylele Sep 7, 2019

Choose a reason for hiding this comment

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

We already loaded the entire NCCH for game list anyway...

Copy link
Contributor Author

@Steveice10 Steveice10 Sep 7, 2019

Choose a reason for hiding this comment

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

I couldn't see a way to get the header data out of the NCCHContainer without making sure Load was called first. And yeah, Load would be called right after the check anyway when the game list reads the program ID.

* @param out_executable Reference to store the executable flag into.
* @return ResultStatus result of function
*/
virtual ResultStatus IsExecutable(u8& out_executable) {
Copy link
Member

@zhaowenlan1779 zhaowenlan1779 Sep 8, 2019

Choose a reason for hiding this comment

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

This should likely be a bool

@Steveice10
Copy link
Contributor Author

Steveice10 commented Sep 8, 2019

Changed the output type to a bool.

@vitor-k
Copy link
Contributor

vitor-k commented Sep 11, 2019

So, this should close #4565 right?

One question, is there any situation where the presence of those files may be important information to the user?
If so, a way to show their presence (but disabled/not launch-able) might be nice, but could wait for a future PR.

@Steveice10
Copy link
Contributor Author

Steveice10 commented Sep 11, 2019

I believe so, yes. And the only thing that immediately comes to mind would be allowing users to view included game manuals, but that should probably be exposed through some other means than an entry in the game list, and would be a feature for another PR.

@jroweboy jroweboy merged commit 4b05078 into citra-emu:master Sep 17, 2019
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants