Skip to content

Classifier#10

Open
sharwell wants to merge 5 commits into
NoahRic:masterfrom
sharwell:classifier
Open

Classifier#10
sharwell wants to merge 5 commits into
NoahRic:masterfrom
sharwell:classifier

Conversation

@sharwell
Copy link
Copy Markdown
Contributor

@NoahRic
Copy link
Copy Markdown
Owner

NoahRic commented Feb 17, 2013

re: 07cfcd8, the items that aren't currently configurable are that way for a reason. They are either ones that shouldn't really be configurable (bold = bold, italics = italics) or have properties that the Fonts and Colors dialog can't do anything about (font, font size, font characteristics). I don't think they should be exposed, especially since, as you note in #7, this doesn't actually fix the bug.

(the "fixes #7" should probably be removed from the description for that reason)

If I don't take 07cfcd8, then the only other interesting one is ebec22c. I only have one small nitpicky comment there (capture tokenIndex as (int)tokenType once in GetClassificationTypeForMarkdownToken), and the other comment I had would go away if the other changes aren't merged (mismatches between e.g. UrlInline and InlineUrl), but it seems like a positive change on the whole. I doubt this will make an appreciable performance difference, but the code you wrote is definitely better.

@sharwell
Copy link
Copy Markdown
Contributor Author

I crossed out rather than remove the reference to issue 7 since the link GitHub added is permanent (removing the text will leave the link to the issue in place, but then no one will know why the link is there).

Users may still want to customize the colors related to items like bold/italics, which is why I think it makes sense to leave those customizable. I certainly can't see it hurting anything to leave users the option.

@andy-uq
Copy link
Copy Markdown

andy-uq commented Jul 16, 2013

I merged this into my fork because I use a black background in my editor, and I was unable to change the colour of link urls from their default blue, which was far too low in contrast and was unreadable.

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.

Block font could do better than Courier New

3 participants