-
-
Notifications
You must be signed in to change notification settings - Fork 63
Add 11 modern color scheme skins for MC #4775
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
base: master
Are you sure you want to change the base?
Add 11 modern color scheme skins for MC #4775
Conversation
3919e49 to
2df7167
Compare
|
Please remove *.po files from your commit. po-files are updated from Transifex. |
2df7167 to
83c0e96
Compare
|
Thanks for you review. Changes made under: 83c0e96 |
zyv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, thank you for the contribution! Cheers to Jindrich N. if you happen to work with him :)
I haven't tried these skins live, but I have checked them quickly for non-contrast colors with our new skin browser:
https://skins.midnight-commander.org
I would otherwise complain about the colors, because my understanding is that these are color schemes that are used like that in other software and people like them, so they should be taken as is. But invisible text somehow I think it better be addressed.
I think most skins are unproblematic, but I have found two exceptions. Could you please find a tweak that works?
catppuccin-latte
catppuccin
Also, a general question: you have added a test plan in the PR. Did you actually perform it?
|
We have loads and loads of 256-color skins and hardly any truecolor ones. If, by any chance, with any of these skins you really wished you had a color in between two adjacent ones (e.g. most likely a darker red, green or blue than the darkest available in the 6x6x6 color cube) then I encourage you to convert that skin to a truecolor one :-) If you're happy with the colors you have now then please ignore this comment. |
|
Thank you for the detailed review! I've addressed the visibility issues you identified: Changes made:
The changes ensure that all text remains visible and maintains good contrast. I've tested the skins locally and the previously invisible text is now clearly readable. Commit: b0f6622 |
|
Regarding your question about the test plan: Yes, I have performed the testing as outlined in the PR description. All 11 skins have been tested with:
The skins work correctly across different terminal emulators and maintain good readability in various lighting conditions. Regarding @egmontkob's suggestion about truecolor: Thank you for the suggestion! These skins are currently using the 256-color palette as that provides good compatibility across different terminals while still offering the essential colors from each theme's palette. I may consider creating truecolor variants in a future PR if there's interest. |
|
Thank you @zyv for the additional review! I've addressed all three points: Changes in commit 524ba6d:1. Added trailing newlines ✅All skin files now have proper trailing newlines for consistency with existing skins. 2. Renamed to follow 256-color convention ✅All new skins have been renamed to follow the *256.ini naming pattern:
3. Updated to fancy UTF-8 characters ✅All widget sections now use UTF-8 symbols consistent with other 256-color skins:
The Makefile.am has also been updated to reflect the new filenames. All skins now follow the established conventions for 256-color themes. |
- catppuccin-latte.ini: Fixed dhotfocus and changedline with same fg/bg colors - catppuccin.ini: Fixed editlinestate and changed with same fg/bg colors - tokyo-night.ini: Fixed changedline with same fg/bg colors These changes address the visibility issues identified in PR MidnightCommander#4775 review where text was invisible due to identical foreground and background colors.
- Fixed invisible text in catppuccin-latte menu sections by adjusting background colors - Fixed low contrast in catppuccin editor and diffviewer sections - Addresses PR MidnightCommander#4775 review comments about non-contrast colors
Does this changes address the contrast? |
- catppuccin-latte.ini: Fixed dhotfocus and changedline with same fg/bg colors - catppuccin.ini: Fixed editlinestate and changed with same fg/bg colors - tokyo-night.ini: Fixed changedline with same fg/bg colors These changes address the visibility issues identified in PR MidnightCommander#4775 review where text was invisible due to identical foreground and background colors. Signed-off-by: Rafael Zago <rafaelvzago@gmail.com>
- Fixed invisible text in catppuccin-latte menu sections by adjusting background colors - Fixed low contrast in catppuccin editor and diffviewer sections - Addresses PR MidnightCommander#4775 review comments about non-contrast colors Signed-off-by: Rafael Zago <rafaelvzago@gmail.com>
524ba6d to
050942d
Compare
zyv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Rafael, thanks for getting back to us!
It's always nice when submitters adjust the PRs and don't disappear after the first comment, as it unfortunately often happens.
I think the contrast is now good for the main parts. I'm still not very happy about the diff viewer in some skins:
catppuccin
matte-black
osaka-jade
ristretto
rose-pine
tokyo-night
Is there any chance you can adjust those to be a bit less toxic / have more contrast?
I know that diff viewer is by far not the most used part of mc, but I really like your work and it would be great if the skins look uniformly good...
|
Hi! Thank you so much for the detailed feedback and the kind words about the work! I really appreciate you taking the time to review these skins thoroughly. What I've fixed:
The new approach:
I've tested all the skins locally and the diff viewer is now much more pleasant to use - no more eye-straining bright backgrounds! The contrast should be excellent across all the modern skins while still preserving each theme's unique identity. Thank you again for your patience and constructive feedback. It's reviewers like you who help make open source projects better! Please let me know if there's anything else that needs adjustment. Best regards, Rafael |
- catppuccin-latte.ini: Fixed dhotfocus and changedline with same fg/bg colors - catppuccin.ini: Fixed editlinestate and changed with same fg/bg colors - tokyo-night.ini: Fixed changedline with same fg/bg colors These changes address the visibility issues identified in PR MidnightCommander#4775 review where text was invisible due to identical foreground and background colors. Signed-off-by: Rafael Zago <rafaelvzago@gmail.com>
- Fixed invisible text in catppuccin-latte menu sections by adjusting background colors - Fixed low contrast in catppuccin editor and diffviewer sections - Addresses PR MidnightCommander#4775 review comments about non-contrast colors Signed-off-by: Rafael Zago <rafaelvzago@gmail.com>
943ee62 to
55a9762
Compare
…r mc Add contemporary color schemes including Catppuccin (latte/mocha), Everforest, Gruvbox, Kanagawa, Nord, Rose Pine, Tokyo Night, and others. Update Makefile.am to include new skin files in distribution. Signed-off-by: Rafael Zago <rafaelvzago@gmail.com> Signed-off-by: Yury V. Zaytsev <yury@shurup.com>
…r mc Add contemporary color schemes including Catppuccin (latte/mocha), Everforest, Gruvbox, Kanagawa, Nord, Rose Pine, Tokyo Night, and others. Update Makefile.am to include new skin files in distribution. Signed-off-by: Rafael Zago <rafaelvzago@gmail.com> Signed-off-by: Yury V. Zaytsev <yury@shurup.com>
d01f717 to
b2ef000
Compare
|
/rebase |
b2ef000 to
c171044
Compare
…r mc Add contemporary color schemes including Catppuccin (latte/mocha), Everforest, Gruvbox, Kanagawa, Nord, Rose Pine, Tokyo Night, and others. Update Makefile.am to include new skin files in distribution. Signed-off-by: Rafael Zago <rafaelvzago@gmail.com> Signed-off-by: Yury V. Zaytsev <yury@shurup.com>
c171044 to
ea820be
Compare
…r mc Add contemporary color schemes including Catppuccin (latte/mocha), Everforest, Gruvbox, Kanagawa, Nord, Rose Pine, Tokyo Night, and others. Update Makefile.am to include new skin files in distribution. Signed-off-by: Rafael Zago <rafaelvzago@gmail.com> Signed-off-by: Yury V. Zaytsev <yury@shurup.com>
|
Another rebase and update, this time for #3531 / PR #4938. The newly introduced As a side note, similarly to #4775 (comment), I'd appreaciate if more skins used bold and underline for what was originally intended to be bold and underlined, rather than (or in addition to) different colors. But I leave this decision to the skin authors. |
ea820be to
2d900b2
Compare
…r mc Add contemporary color schemes including Catppuccin (latte/mocha), Everforest, Gruvbox, Kanagawa, Nord, Rose Pine, Tokyo Night, and others. Update Makefile.am to include new skin files in distribution. Signed-off-by: Rafael Zago <rafaelvzago@gmail.com> Signed-off-by: Yury V. Zaytsev <yury@shurup.com>
Really did that this time. |
…r mc Add contemporary color schemes including Catppuccin (latte/mocha), Everforest, Gruvbox, Kanagawa, Nord, Rose Pine, Tokyo Night, and others. Update Makefile.am to include new skin files in distribution. Signed-off-by: Rafael Zago <rafaelvzago@gmail.com> Signed-off-by: Yury V. Zaytsev <yury@shurup.com>
2d900b2 to
ad4f689
Compare
…r mc Add contemporary color schemes including Catppuccin (latte/mocha), Everforest, Gruvbox, Kanagawa, Nord, Rose Pine, Tokyo Night, and others. Update Makefile.am to include new skin files in distribution. Signed-off-by: Rafael Zago <rafaelvzago@gmail.com> Signed-off-by: Yury V. Zaytsev <yury@shurup.com>
ad4f689 to
cb77953
Compare
|
Another rebase and update, this time to add |
|
@rafaelvzago I'm wodering, are you planning to return to this issue and address the feedbacks? For the time being, I keep updating these skins according to upstream changes. But I won't do this endlessly. As far as I recall, the main criticized points were:
but by all means refer to the detailed conversation, there might be more. Back to the issue of exact colors for a second: Since you started to work on these, truecolor support landed also for mc's ncurses build. Most of the graphical terminals do support them nowadays (and there are some that don't, but at least recognize the sequences and approximate to the nearest entry of the 256-color palette – make sure you're not using one of these for testing). You shouldn't worry about and shouldn't be held back by technical limitations; accurate representation of these colors, and harmony with non-terminal-based apps using the same palettes should be more important. I'd also like to draw your attention to the alias feature of skin files. It might help a lot while developing and fine-tuning these skins to define aliases using the palette author's chosen names, and later on rely on those names rather than having to repeat the exact RGB color. E.g.: Maybe you could even end up with a set of 4 cappuccin, or 3 rose-pine etc. skins that only differ in the alias definitions of these colors, according to how these theme variations are defined by their designer. There's also a new and up-to-date README.txt next to the skins in the source, and the online skin editor has also been revamped (although it still doesn't showcase many of the colors, this still needs to be improved). @zyv @mc-worker I don't know what's your take on this, and if you care about such formality; you've both approved this change, but I think they do need more work to be accepted; I'd revoke those approvals for now. |
I've dismissed both reviews as obsolete for now. |
I think it's very unfortunate that the author apparently has lost interest in the PR. I very much like the idea of having a modern set of skins based on widely accepted color palettes. As you mention, it would be particularly awesome to have much more True Color skins, of which we only have your demo skins right now. It also makes developing these skins easier, because as you said, we can directly use "official" colors. On the other hand, I'm very much suffering to accept a large amount of skins with known and obvious contrast problems / toxic color combinations that can be easily avoided. It feels to me much more like a disservice to the users and at the moment I'd be more inclined to let it rot if nobody cares about it rather than just merge it as is. To top it off, there is no chance I can make time to fix this myself... or else we'll never see a release at all. |
Yup, that's my take, too.
It's not just about time. It's about anyone else not having the same artistic vision as the original author. It's the kind of task that even if I had infinite amount of time, I'd prefer the original author to complete. That being said, time is also an issue. Developing a good skin takes a lot of time. I'm still hoping OP will come back one day and continue the work – these are really promising skins! |
Well, I think it's a bit less critical in this particular case, if we define the goal to skin mc in those widely used palettes. Of course, it still leaves a lot of room for individual artistic vision, but there are many existing skins for other software that one can take inspiration from. |
If the original author doesn't but someone else does come along and completes these skins, I'd be happy to see them land! (I guess I shouldn't have mentioned the artistic vision side of the story, and I also worded it poorly: I personally would prefer not to do this for this reason, but if someone else does, I'm fine. Sorry for this tiny bit of unnecessary confusion.) |


Summary
Test plan
mc -S <skin-name>New skins added