Ticket #10, #11, #12: semi automated screenshots and fix attribute inheritance #22
Conversation
zyv
left a comment
zyv
left a comment
There was a problem hiding this comment.
Just a few comments - not a review (my understanding is it wasn't requested either). Looks very impressive!
| @@ -0,0 +1,109 @@ | |||
| This file is used to take a screenshot of mcdiff. | |||
There was a problem hiding this comment.
Just a thought - maybe you could put this inside a multiline comment, just to make the file a valid C code.
I understand that this doesn't matter, but you know the mcedit parser is not particularly good.
I'm afraid that this way you can unintentionally introduce incorrect highlighting.
There was a problem hiding this comment.
According to the instructions, this file is not opened in mcedit.
Moreover, mcedit has special hacks for syntax highlighting, it's a mixture of colors defined in the skin and colors defined for the syntax. Neither the screenshot taking patch, nor the online skin viewer supports it.
So we cheat a little bit and intentionally avoid showing such screenshots.
Maybe we'll fix this one day. Until then it's perfectly okay (and even has an additional step of "protection") to not detect the file as C source code.
There was a problem hiding this comment.
Ah, okay. Thank you for the explanation. As I said, I didn't realize you wanted a review and only glanced at the instructions instead of reading them more attentively.
You are right, it's fine to leave it as it is. I would have made it a comment out of principle as a matter of taste, but if you don't like it, you can ignore my suggestion.
| @@ -0,0 +1,82 @@ | |||
| This file is used to take a screenshot of mcdiff. | |||
There was a problem hiding this comment.
Same here, although it probably matters even less.
Please review properly -- I accidentally forgot to press that button :) |
egmontkob
force-pushed
the
10_11_12_semi_automated_screenshots_and_fix_attribute_inheritance
branch
from
f5cebad to
794a56f
Compare
|
Added a comment to Renamed |
zyv
left a comment
zyv
left a comment
There was a problem hiding this comment.
I really like this. It's fantastic! It would be cool if you could showcase the following items:
hiddenfiles-hide-charfilename-scroll-left-char
Is it just a matter of adding the hide files shortcut and scrolling right a couple of times to the instructions?
…enshots Document how to recreate screenshots that are very close to (but not necessarily identical) to the current ones. Signed-off-by: Egmont Koblinger <egmont@gmail.com>
…: Create new screenshots Fix the HTML side of attribute inheritance and faulty colors. Increase screenshot height from 30 to 32 rows, to allow more room for file type highlights. These screenshots are not compatible with the current rendering engine, the next commit will fix that. Signed-off-by: Egmont Koblinger <egmont@gmail.com>
…ndering engine Update the skin rendering engine to match the HTML + CSS of the new screenshots. Fix the JS side of attribute inheritance. Signed-off-by: Egmont Koblinger <egmont@gmail.com>
egmontkob
force-pushed
the
10_11_12_semi_automated_screenshots_and_fix_attribute_inheritance
branch
from
794a56f to
dcf8c30
Compare
It's already there in the "brief" screenshot. It truly surprised me that showing / hiding hidden files, even though has a button at the top-right corner of each panel, is a global (not per-panel) property. Is this a bug or feature? In the current skin viewer's first screenshot, one of the panels has dotfiles enabled and the other has them disabled. This either originated from an older version of mc that behaved this way, or was manually manipulated and shows an impossible state (something I no longer want to have, manually modifying those files looks like nightmare). If it's a bug then we should first fix that and then recreate the screenshots (and update the instructions).
Yup, done. There are still many colors not showcased in the screenshots. I'll add a bug soon. Awesome new feature: Move the mouse somewhere over the screenshot and wait for the tooltip text. It tells you which color (and if relevant, which character) of the skin file is used in that cell. |
Ah, I didn't realize that. Awesome!
I think this must be a feature, since it works in FAR in the same way.
Yeah, I think I didn't realize it was on brief (or maybe it wasn’t), and hand-patched it in there. |
egmontkob
deleted the
10_11_12_semi_automated_screenshots_and_fix_attribute_inheritance
branch
2 participants
This is the main thing I wanted to do.
The screenshots are now easy to generate, easy to reproduce similar ones. No more manual editing of those templates!
Many incorrect colors have been fixed. Some were due to faulty inheritance / fallback, some were presumably due to manual template editing mistakes.
I'll continue with many smaller improvements / addressing the other issues, but I wanted to send out this big step in the mean time :)