Make blank space less apparent. #2501
Conversation
|
Left the old syntax highlighting, right the new. Top default Spyder scheme, bottom Spyder/Dark scheme. Especially the spaces in 'real' code improve readability, like the ones around the equal signs. Apparently something goes wrong with the spaces after the tab character, which I only noticed when making the screenshots.
|
|
The issue with the tab character has been fixed. Not going to make new screenshots because it is bedtime here. Perhaps the code can be improved by matching a white space regexp instead of reformatting one character at a time using a double loop. But it works and isn't that bad IMO. |
|
@hugobuddel, thanks a lot for taking the time to work on this :-) @goanpeca, would you mind to review this PR? |
| fr*color_foreground.red() + (1-fr)*color_background.red(), | ||
| fg*color_foreground.green() + (1-fg)*color_background.green(), | ||
| fb*color_foreground.blue() + (1-fb)*color_background.blue(), | ||
| 255, |
There was a problem hiding this comment.
Does it work to only changing the alpha value ? This would require less computations.
|
Indeed @Nodd, now setAlpha and a whitespace regular expression is used. Now it is cleaner, more uniform in code style and probably useful if we ever would like to show new-line characters for some reason. By the way, funny that |
|
Looks good :-) 👍 |
| @@ -414,6 +414,20 @@ def highlightBlock(self, text): | |||
|
|
|||
| match = self.PROG.search(text, match.end()) | |||
|
|
|||
| # Make blank space less apparent by setting the foreground alpha. | |||
| # This only has an effect when "Show blank space" is turned on. | |||
| re_blank = re.compile("\s+") | |||
There was a problem hiding this comment.
This line should perhaps be a class variable (look on top for)
# Syntax highlighting rules:
PROG = re.compile(make_python_patterns(), re.S)
IDPROG = re.compile(r"\s+(\w+)", re.S)This way the compilation is done only once when the class is instantiated, and the code would be more uniform, as you can see no regex is compiled within the highlighting method.
|
Thinking a bit more about this, this code should work in the other syntax highlighters, so perhaps it could be refactored as a method inside the |
|
Ah, About the hard-coded value: it seems to 'expose' too much of the internal working of QColor. Perhaps it would be nicer to multiply with a factor, like: Or perhaps indeed with a setable class variable. Although personally I don't think it would be worthwhile to provide a user interface for it until someone asks for it. |
| while match: | ||
| start, end = match.span() | ||
| start = max([0, start+offset]) | ||
| end = max([0, end+offset]) |
There was a problem hiding this comment.
In which cases start+offset or end+offset could be negative ?
There was a problem hiding this comment.
start+offset and end+offset are negative when a line in a multiline string starts with spaces.
That code is copied from the loop earlier in the function. Interpreting the code (and some experimentation) taught me that highlightBlock() is called for each line of text, which might be part of a multiline string. The relevant quotes are temporarily prefixed to the line. The number of added characters is tracked by the negative valued offset variable.
|
Great. Is not about providing an user interface, is to keep the code cleaner I guess... and yes, a factor seems better. |
|
I like the approach but it would only seem to work on python files so far, isnt it? |
| @@ -414,6 +414,20 @@ def highlightBlock(self, text): | |||
|
|
|||
| match = self.PROG.search(text, match.end()) | |||
|
|
|||
| # Make blank space less apparent by setting the foreground alpha. | |||
There was a problem hiding this comment.
All this should have an if statement tied to the option of showing spaces, no?
Also to make it work with other syntax highlighters it should be refactored as a separate method inside BaseSH
|
Now the less pronounced space highlighting should work with all file types. However, I've only checked The code is moved to A check for shown spaces isn't necessary, because normal space and tab glyphs have no content. So it doesn't matter if we change the alpha value of those. |
|
@hugobuddel thanks for all the updates :-) great work. What I meant from, the check is to avoid unnecessary calculations if the option is not selected, cause even if it does not affect the look it will add an overhead that could be easily checked, dont you think? |
|
About PEP 8 consistency, we follow it for everything except when Qt is involved, so actually https://github.com/spyder-ide/spyder/wiki/Dev%3A-Coding-Style |
| @@ -187,10 +191,28 @@ def set_color_scheme(self, color_scheme): | |||
| self.color_scheme = color_scheme | |||
| self.setup_formats() | |||
| self.rehighlight() | |||
|
|
|||
There was a problem hiding this comment.
There doesn't seem to be a consensus about spaces on otherwise empty lines, at least not in syntaxhighlighters.py. If anything, it is to keep those spaces.
Personally I like the indentation of empty lines to match the indentation of the next non-empty line. That is the easiest to read with 'show blank space' in my opinion, because it visually groups code that belongs together. Also it prevents the cursor from needlessly jumping around horizontally when using the up/down arrows to move.
Nevertheless, I'll change it. My editors usually add these spaces (which I like) but I remove them before committing if it is a modification of existing code. This one apparently slipped through. (I even kept the line in syntaxhighlighting.py that had just one single space until I couldn't stand it anymore a few commits ago..)
|
@hugobuddel indeed most of the spyder code base has a lot of blank lines with spaces due to autoindentation in editors etc... but we are trying to fix this on every PR that is released. There is also a pending PR that will most likely erase this blank lines (to be done still!). |
|
A personal note about 'fixing' the whitespace created by autoindentation in editors. The whitespace in spyder will not affect me much because I'll be mainly using spyder on my own code. Nevertheless, my motivation for the changes in this pull request seem to conflict with the spyder whitespace policy, so it is good to be aware of this discrepancy even though it probably will not matter much in practice. For me, empty lines matching the indentation level of the lines of code are
From that perspective it is not a fix to remove spaces from otherwise empty indentation lines. A fix would be to match the indentation of the next line. The goal of this comment is to give you some more information to make the best decision, which very well might be to remove the lines anyway. I'll adhere to whatever standard spyder will chose in future commits. |
| self.setFormat(start, end-start, color_foreground) | ||
|
|
||
| match = self.BLANKPROG.search(text, match.end()) | ||
|
|
There was a problem hiding this comment.
@hugobuddel, thanks for the change, it is a very nice work and as soon as the PR is merged I will leave the option ON, but I think we can refactor the code to make this better (the new code is almost an exact copy of the base class, but with a couple of extra lines!)
It would be much better in this case to write an new method in the base syntax highlighter that returns True (or False) by default and gets overloaded in the child syntax highlighters classes as needed
# in the base class
def is_inside_string(self): # Random name ... if you think of a better one please go ahead
return True # or False...
# in the child highlighter class... python in this case
def is_inside_string(self):
"""Overloaded method to account for all the different python strings."""
inside_string = self.previousBlockState() in (
self.INSIDE_SQSTRING,
self.INSIDE_DQSTRING,
self.INSIDE_SQ3STRING,
self.INSIDE_DQ3STRING,
)
return inside_stringThis way we avoid duplicate code and make it more future proof.
Cheers ;-)
There was a problem hiding this comment.
And then of course you could add your new code inside the original highlight_spaces method.
|
Thanks @goanpeca, it is now refactored in an even better way. I didn't like the duplication of code, and indeed though of generalizing the 'in a string' part, but disliked that as well. You showed my why by taking the time to write it out: are there actually other use cases that resemble the Python multiline docstring like the earlier screenshot? I can't think of many. Instead, it was simpler to just recolor leading spaces, which is easy to generalize. And, it was trivial to add support for trailing spaces as well! The leading space format is hardcoded to 'normal', which is fine I guess. But the trailing space format is now hardcoded to 'number', because that is dark red in the default scheme. Any suggestions on how to improve on that?
|
| start, end = match.span() | ||
| start = max([0, start+offset]) | ||
| end = max([0, end+offset]) | ||
| if end == len(text) and format_trailing is not None: |
There was a problem hiding this comment.
Much better, maybe just add a comment on top of this line?
# Formats points at the start of the line... leading points
|
I think that both trailing and leading points should be normal (color) for now. |
|
Added some comments and used normal format for trailing spaces. Perhaps we can support a different format for trailing spaces in the future, the possibility now exists. I'd say the PR is once again as good as it should be! |
|
Review done! @Nodd can you take a quick look? |
|
@ccordoba12 review done! |
| Derived classes could call this function at the end of | ||
| highlightBlock(). | ||
| """ | ||
| show_blanks = self.document().defaultTextOption().flags() & QTextOption.ShowTabsAndSpaces |
There was a problem hiding this comment.
This line is probably longer than 79?
I am actually flexible on that PEP8 rule but I think @ccordoba12 is more serious about it :-)
|
These lines indeed were a bit long and are split up. The code is perhaps easier to understand now by using extra variables. |
| self.setCurrentBlockState(state) | ||
|
|
||
| # Use normal format for indentation and trailing spaces. | ||
| self.formats['leading'] = self.formats['normal'] | ||
| self.formats['trailing'] = self.formats['normal'] |
There was a problem hiding this comment.
@hugobuddel is this only applied to Python then?
There was a problem hiding this comment.
Yes on purpose @goanpeca, because it seems that it is not useful in any of the other syntax highlighter classes. It only recolors the green indentation (and trailing) spaces of multiline strings to the 'normal' format and none of the other classes have something similar: they already color all leading and trailing spaces with the 'normal' format. Or they already have highlighting rules that shouldn't change, like multiline C++ comments or the Fortran rules. I probably missed some cases though and better be safe than sorry and default to not changing them. (Edit: typo.)
|
Got it, thanks for your work on this |
|
@goanpeca, is this ready for merge? |
|
It is working as expected so I would say yes (unless you find something weird in the code ;-) ) |
|
Since you reviewed this one, please do the honors of pressing the green button ;-) |
|
👍 |
Make blank space less apparent.
|
Thank you @goanpeca @ccordoba12 and @Nodd , it was great working with you. The patch got much better through our conversation! |
|
@hugobuddel thanks for your work on it :-), it was an issue bothering a lot so of people. It does look much better now. We are always happy to have new contributors, and hopefully you can continue helping us improve Spyder. Cheers |
|
Perhaps I will contribute more, this was just scratching my own itch :-). So far I like spyder, and I'll try to use it for more projects. All this space highlighting stuff made me realize that it must be possible to do better than such ASCII-based code editing. It seems archaic. Like the 79 character limit: I agree that it is a good style guide and try to follow it, but it feels bad that it is a necessity at all! Why should we as programmers worry about things like spaces and line breaks? It must be possible to decouple the presentation of the code from how it is stored. We shouldn't need a PR like this at all. Achieving such a separation of presentation is a whole other discussion. And it is probably not yet done because it is too hard to get right. Nevertheless, perhaps I'll try to see how far current tools make this possible already now. |
|
We all start with own itches ;-) I am not following you, but my take on this is that all theses are about being consistent and at the end code is more read than written and having code written in a consistent manner can take projects farther. Having a set of rules (even if some might feel archaic) helps in having have a cleaner codebase, helps people reading the code know what to expect, and helps developers in the process. |
|
Yes I fully agree that the rules are necessary. And consistency is more important than having perfect rules. Because as you say, it improves collaboration by having easier to understand code. Perhaps I'll need some more time to figure out what my 'thesis' actually is :-). This thread helped me with my thought process, which is why I mentioned it, but it is not the right place to really discuss it. It's that our tools (and perhaps languages themselves) could be so much better in assisting us with writing/reading code than they are now. My feeling is that in a perfect tool it wouldn't be necessary or desirable to 'show spaces' at all, even to the extent that it would be conceptually meaningless to do so. But I'll stick with spyder; perfect tools don't exist and spyder is a great leap towards better tools in many ways. Hopefully someone will one day revert this merge because it has become useless :-). |
|
👍 |
Less apparent blank space, improved readability, fixing #2175.
Foreground color of blank space (if shown) is blended with the background color, therefore being less strongly present in both light and dark schemes.