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

Cache parsed W3C colors #3983

Draft
wants to merge 2 commits into
base: v2_develop
Choose a base branch
from

Conversation

TheTonttu
Copy link
Contributor

I wasn't sure if the ColorStrings is meant to be thread-safe or not but just to be sure I used concurrent dictionary to at least keep it internally consistent.

Also I'm not sure if the cache should be purged in case the UI culture is changed. I added TODO comment about it. In general it is quite error prone to switch the current UI culture on-the-fly anyways so I'm not sure we want to open that can of worms. In case it needs to be supported, I think I have a somewhat simple solution to it but I would first need to probably create microbenchmarks for it.

Fixes

Proposed Changes/Todos

  • Cache parsed W3C colors in ColorStrings

Pull Request checklist:

  • I've named my PR in the form of "Fixes #issue. Terse description."
  • My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
  • My code follows the Terminal.Gui library design guidelines
  • I ran dotnet test before commit
  • I have made corresponding changes to the API documentation (using /// style comments)
  • My changes generate no new warnings
  • I have checked my code and corrected any poor grammar or misspellings
  • I conducted basic QA to assure all features are working

Allocations

String and DictionaryEntry allocations dropped nicely.

Before After
01 object-allocations before 02 object-allocations after
01 dictionary-entry before 02 dictionary-entry after

@TheTonttu TheTonttu requested a review from tig as a code owner March 12, 2025 21:25
@tig
Copy link
Collaborator

tig commented Mar 13, 2025

The ContextMenus scenario switches culture FWIW

@TheTonttu
Copy link
Contributor Author

The ContextMenus scenario switches culture FWIW

sigh I'll mark this as draft and open the can of worms...

@TheTonttu TheTonttu marked this pull request as draft March 13, 2025 16:17
@TheTonttu
Copy link
Contributor Author

@tig On second thought are there any plans to actually support localized names for the W3C color names?
If not then I'm not sure if there is any point to add UI culture change detection and complicate the implementation further.

At the moment there are only en-US/neutral strings for the W3C colors.

As far as I know, the W3C color names are meant to have standardized color names for design/development purposes, so I'm not sure it makes any sense to pass the localized names past the UI level. Ideally the localized name would be first "neutralized" before passing it further down the system so culture-specific quirks are avoided.

@tznind
Copy link
Collaborator

tznind commented Mar 13, 2025

for w3c names they should not be translated imo. Its like a ux enum for designers and web devs - should be consistent

@TheTonttu
Copy link
Contributor Author

TheTonttu commented Mar 13, 2025

Figures, now I'm more confused and wondering why this whole thing is not just a static class containing named Color instances (static readonly fields) supported by enum mapping. The enum then could be further mapped to name/localization strings if really needed.

At least at quick glance, it seems odd to go through all the hoops to parse the named colors through resource strings if localization is not needed. I'm most likely missing some critical information.

@tznind
Copy link
Collaborator

tznind commented Mar 13, 2025

I'm all for simplicity, thoughts @tig? switching the w3c names out of resx?

@tig
Copy link
Collaborator

tig commented Mar 13, 2025

I don't remember. I was probably over thinking it.

@BDisp
Copy link
Collaborator

BDisp commented Mar 13, 2025

I'm all for simplicity, thoughts @tig? switching the w3c names out of resx?

I vote to remove it.

@TheTonttu
Copy link
Contributor Author

TheTonttu commented Mar 14, 2025

I'll leave this open for now. Feel free to close the PR if you decide to refactor redesign the W3C color handling.

@TheTonttu
Copy link
Contributor Author

If you don't mind, I could also take a look at the redesign as I'm sure people are already busy with other stuff. I could at least do some sort of rough draft of the new interface(s) and create an issue based on that.

@tznind
Copy link
Collaborator

tznind commented Mar 19, 2025

If you don't mind, I could also take a look at the redesign as I'm sure people are already busy with other stuff. I could at least do some sort of rough draft of the new interface(s) and create an issue based on that.

Absolutely, this would be great :)

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.

4 participants