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

Changeable external icon sets #1817

Merged
merged 18 commits into from Nov 18, 2021
Merged

Changeable external icon sets #1817

merged 18 commits into from Nov 18, 2021

Conversation

Copy link

@ghost ghost commented Nov 16, 2021

Summary

This PR extends 86Box's capabilities by adding dynamically changeable external iconsets.

How to use it:

Open > Tools > Program Settings... > Iconset:

How it saves it:

To 86Box.cfg [General] iconset =

How to add iconset:

1, Convert your icons to .ICO format. You should have 16x16, 20x20, 24x24, and 32x32 sizes at least for DPI-scaling.
2, Name your icons as in src\win\icons. Be sure that you have a replacement for every single icon.
3, Create a new txt file named iconset.txt, and write a single line into it - the name you want to see in the combobox.
4, Choose a short name for your iconset and copy the files to roms\icons\<your chosen name>\.
5, The new iconset should show up in the combobox.

Why roms\icons\ folder?

Because the romset has to be downloaded for every 86Box instances, so this is the easiest way to share extra icons drawn by the community, without compiling them into the program. Also this allows the check and approve any new icon sets, and the path can be redirected with all the existing infrastructure (e.g. command line parameters) for the rom directory. The rom path currently is well definied for Unix too, but introducing a new path for this could lead to complications in this case.

Do we have examples?

For example I'll open two PR's on the romset side to show how these iconsets could be created.

The Legacy iconset is identical to the current one, which hopefully will became really legacy when v3.0 is complete.
The Inspired by WinBox iconset is the one, used in the ImGui branch currently.

Since the extra sets does not have hard infrastructure, hopefully there will be more in the future.

Under the hood:

  • The dialog is reused, and added the new strings by reusing the existing ones.
  • The (Default) string is constructed from "(" + IDS_2090 + ")"
  • The iconsets are explored from the folder returned by plat_get_icons_path(), where a FindFirstFile-FindNextFile-FindClose loop will automatically recognizes all the directories
  • If the txt file exists, it will be opened as text file, and one line will be read from it as text for the combobox.
  • The icon combo has pointers in the CB_ITEMDATA field to strings for the folder name. The display name is constructed from the txt or as fallback from the directory name. The strings are destroyed in WM_DESTROY of the dialog.
  • The (Default) entry is stored as icon_set="" which means, that icons should be loaded from resources, while for the other ones the folder name will be stored as icon_set.
  • The existing icon table was used for the StatusBar, but by adding all the missing icons, so from now the Settings dialog will also use these. plat_clear_icon_set() will destroy the table, and plat_load_icon_set() will fill, according to icon_set.
  • The name of the expected icon files are stored in the icon_files[] table. It can be expanded later when needed.

Notes:

  • You can change the icons dynamically in the folder for developing purposes, but you have to reload the set from the dialog.
  • It's not necessary to add such a txt file, but without this the folder's name will be displayed.
  • Icons can be missing from your set, but not recommended - in this case the compiled ones will be used instead.
  • The PR can handle if you remove your iconset, but the config still contains it. It'll simply revert to the default one in such cases.
  • The iconinfo.txt can hold more than one lines, for the GUI only the first one matters. For example you can add copyright, license, release date, or you can mention the creators' names, etc. after the first line.

Checklist

  • Closes #xxx
  • I have discussed this with core contributors already
  • This pull request requires changes to the ROM set

References

N/A

@ghost ghost mentioned this pull request Nov 16, 2021
2 tasks
@dhrdlicka
Copy link
Contributor

@dhrdlicka dhrdlicka commented Nov 17, 2021

I am not all that convinced about the actual usefulness of this feature. To be fair it only adds to feature creep and doesn't provide any non-negligible benefits.

src/86box.c Outdated Show resolved Hide resolved
src/include/86box/plat.h Outdated Show resolved Hide resolved
src/include/86box/win.h Outdated Show resolved Hide resolved
src/win/languages/en-US.rc Outdated Show resolved Hide resolved
src/win/win_icon.c Outdated Show resolved Hide resolved
src/win/win_lang.c Outdated Show resolved Hide resolved
src/win/win_ui.c Outdated Show resolved Hide resolved
src/win/win_ui.c Outdated Show resolved Hide resolved
src/unix/unix.c Outdated Show resolved Hide resolved
src/win/win_stbar.c Outdated Show resolved Hide resolved
dhrdlicka
dhrdlicka previously requested changes Nov 17, 2021
src/win/languages/dialogs.rc Outdated Show resolved Hide resolved
Laci bá and others added 4 commits Nov 17, 2021
- Remove icon functions from plat.h.
- Fix some indentation problems.
- Remove unused parameter from win_stbar.c
- Rename win_lang.c to win_progsett.c
- Remove stub functions from unix.c
- Move win_load_icon_set() to ui_init()
- Replace the translated texts to English, and let the translators translate them
- Fix the control IDs in dialogs.rc.
- Use the requested solution in win_icon.c for setting the array variables.
@ghost ghost requested a review from dhrdlicka Nov 17, 2021
@dhrdlicka dhrdlicka dismissed their stale review Nov 17, 2021

feedback addressed

dhrdlicka
dhrdlicka previously requested changes Nov 17, 2021
Copy link
Contributor

@dhrdlicka dhrdlicka left a comment

Marked some slight oversights, otherwise the PR seems to be fine code-wise. I still don't agree with the feature as whole though

src/win/win_ui.c Outdated Show resolved Hide resolved
src/win/win_progsett.c Outdated Show resolved Hide resolved
@ghost ghost requested a review from dhrdlicka Nov 17, 2021
@dhrdlicka dhrdlicka dismissed their stale review Nov 17, 2021

feedback addressed

Copy link
Contributor

@dhrdlicka dhrdlicka left a comment

I don't think I have any new feedback anymore

@ts-korhonen
Copy link
Contributor

@ts-korhonen ts-korhonen commented Nov 17, 2021

I have to agree with @dhrdlicka being critical about the usefulness of this feature to regular users and the worry about feature creep. I understand it could make testing icons easier, but I don't think it's justified only to avoid having to recompile.

@OBattler OBattler merged commit 80fc42d into 86Box:master Nov 18, 2021
4 of 19 checks passed
@ghost ghost deleted the changeable_icon_set branch Nov 19, 2021
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.

None yet

3 participants