Skip to content

Improvements to firmware checks when "System Files are in Content Directory" is enabled#16170

Merged
LibretroAdmin merged 1 commit intolibretro:masterfrom
bslenul:core-info-fw-changes
Jan 28, 2024
Merged

Improvements to firmware checks when "System Files are in Content Directory" is enabled#16170
LibretroAdmin merged 1 commit intolibretro:masterfrom
bslenul:core-info-fw-changes

Conversation

@bslenul
Copy link
Copy Markdown
Contributor

@bslenul bslenul commented Jan 26, 2024

Description

With this PR if System Files are in Content Directory is enabled:

  • A line will now be shown in the core infos, below "Firmware":

    image

  • If content is NOT loaded, firmware files in the core infos will be checked in the "system" folder as usual.

  • If content is loaded, firmware files in the core infos will now be checked in the content folder (see how "scph5501.bin" is now missing compared to the previous screenshot):

    image

  • Logs will now output the content directory as "SYSTEM_DIRECTORY" instead of the global "system" folder. So for example instead of seeing:

    [INFO] [Environ]: SYSTEM_DIRECTORY: "C:\RetroArch-Win64\system"

    you'll see:

    [INFO] [Environ]: SYSTEM_DIRECTORY: "D:\Games\Sony - PlayStation"

  • edit: Core infos will now show the current directory being checked for firmware files.

Also, the regular system dir path is returned without a trailing slash so I added a check for when System Files are in Content Directory is enabled to remove it (because path_basedir() returns a trailing slash). I don't think it was breaking anything before so if this seems superfluous I'll remove it.

Related Issues

Closes #15272

Reviewers

No clue, but please review :D I need some dev brains to make sure I didn't break anything and that I didn't miss/overlook anything.
Tested multiple scenarios on both Windows and Linux and, AFAICT, it works fine but I'd like to be 200% sure :p

@hizzlekizzle
Copy link
Copy Markdown
Collaborator

Would it be possible to add a line after your new note that says "looking in /path/to/system"?

@bslenul
Copy link
Copy Markdown
Contributor Author

bslenul commented Jan 26, 2024

Would it be possible to add a line after your new note that says "looking in /path/to/system"?

I love the idea, PR updated:

Normal:

image

System Files are in Content Directory enabled + no content loaded:

image

System Files are in Content Directory + content loaded:

image

@bslenul bslenul force-pushed the core-info-fw-changes branch 3 times, most recently from f351892 to d916cbb Compare January 27, 2024 00:58
@sonninnos
Copy link
Copy Markdown
Collaborator

Good stuff. Might be even better if they would stand out more with some other prefix than the same (!)..

@bslenul
Copy link
Copy Markdown
Contributor Author

bslenul commented Jan 27, 2024

Thank you! And yeah I agree, just tested and it's already much more noticeable with a simple "-" instead for example:

image

Unless you had something specific in mind?

@bslenul bslenul force-pushed the core-info-fw-changes branch from d916cbb to d21adbe Compare January 27, 2024 10:06
@LibretroAdmin
Copy link
Copy Markdown
Contributor

@sonninnos Are you satisfied with the PR in its current state?

@sonninnos
Copy link
Copy Markdown
Collaborator

LGTM.

@bslenul bslenul force-pushed the core-info-fw-changes branch from d21adbe to ce00792 Compare January 28, 2024 15:52
@bslenul
Copy link
Copy Markdown
Contributor Author

bslenul commented Jan 28, 2024

Really minor update to use content_get_flags() here:

      uint8_t flags                   = content_get_flags();
      bool content_is_inited          = flags & CONTENT_ST_FLAG_IS_INITED;

instead of:

      content_state_t *p_content      = content_state_get_ptr();
      bool content_is_inited          = p_content->flags & CONTENT_ST_FLAG_IS_INITED;

It doesn't change anything to the result but I think it's a bit more readable and it's also more consistent with the rest of the code in menu_displaylist.c where _get_flags() is usually used.

Copy link
Copy Markdown
Contributor

@LibretroAdmin LibretroAdmin left a comment

Choose a reason for hiding this comment

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

I left some feedback on things I'd like to see addressed

Comment thread runloop.c
strlcpy(tmp_path, fullpath, sizeof(tmp_path));
path_basedir(tmp_path);

/* Removes trailing slash */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK, you can do one of two possible improvements here:

  • (Solution 1) Either make it so that you only do strlen(tmp_path) once, having it do it two times like this is wasteful
  • (Solution 2) (Preferred but you will have to see how achievable this is) Not having to use the strlen at all and being able to figure out the size some other way beforehand. But this might not be achievable in which case you can go for solution 1.

Copy link
Copy Markdown
Contributor Author

@bslenul bslenul Jan 28, 2024

Choose a reason for hiding this comment

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

PR updated, I couldn't find a way without strlen so I went for solution 1, hope this is OK.

Comment thread menu/menu_displaylist.c Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature Request] "System Files are in Content Directory" improvements

4 participants