Non printable characters #2350
Non printable characters #2350
Conversation
|
@AghaSaad04 Can you change that first DEMO image to without using the Grammerly plugin? (Just disable it then screenshot again) |
|
Done @thadguidry |
|
Ah, we also really needed this in the Project preview (prior to Create Project)
|
|
This is a good start, but it's not fully mapped yet? I typed in Control Chars using Windows Key + ALT + numpad 0001 - 0031 on various lines inside Notepad++ on Windows 10. (You can't do 0000, that's True Null so don't worry about it and where NULL injection security issues comes from by the way :-) SOURCE:
RESULT: |
| @@ -356,3 +356,16 @@ ul.sorting-dialog-blank-error-positions > li { | |||
| cursor: move; | |||
| .rounded_corners(3px); | |||
| } | |||
|
|
|||
| .interfaceForTextArea{ | |||
| font: 400 13.3333px Arial; | |||
There was a problem hiding this comment.
Hmm, wondering if we should actually prefer first some Unicode fonts (Arial Unicode,etc.) in preference order, and then Arial ? Do popular Linux distros have them? What about Mac?
|
@thadguidry Firstly thanks for reviewing, and yeah it is not fully mapped yet. That was a sort of prototype, I just wanted to make sure that the feature I was implementing was what you guys wanted. I will look into the Project preview part. Hopefully, that would not be tough. I will fix that typo. :) |
|
@AghaSaad04 Great! Understood this is a prototype to see what was possible. Now that we know a bit more, we can begin to make improvements. To reduce repetition, I'd improve the code through the use of |
|
Yeah, @thadguidry. Working on it to make it efficient and optimize. |
|
@AghaSaad04 I added my simple test file above the image in previous comments, if you want to use it or adapt it for test cases, up to you. |
|
Perfect Thanks @thadguidry. Imported the test file: Result: |
|
@AghaSaad04 Would a dictionary make things easier? UPDATE: nevermind, the mapping is implied through the array index of 0-31. You and I know that but others will have to figure this out without a bit of code commenting, so... :) |
|
Also, I'd replace char(9) |
|
One last thing, that I think would be useful to users....
Sublime and other IDE's have the ability to toggle rendering white space in preferences. Thoughts? Do you think the existing |
|
Lastly, the label "non-printable characters" is not accurate, since many Unicode chars also do not print with a glyph such as U+A0 and U+200B and U+FEFF , etc. |
|
I would not add a "non printable characters" textbox at this place in the UI, I think it is way too visible. This is not something that people will need to toggle often - better keep that precious space for other things (such as extensions). I think the option should be enabled by default and it should be possible to disable it in some menu, for instance in the preferences. |
|
@wetneb This was discussed before on the issue... Owen and Ettore also thought having a quick toggle was useful. But I understand moving it outside of |
Where? I can't find that in #1286. |
|
@wetneb maybe was another related issue or the mailing list. We can ask them to comment in here. Anyways, I think much more useful will be #2351 for most users. But then after using the proposed facet, they will want to quickly visualize those control codes in their rows because they will be asking..."really? I have control codes in those rows? WHERE? show me." |
@thadguidry I made some changes and got these results. But I am still trying to figure out when a user switches to another parsing format, it has to be unchecked and then checked again for the functionality to work. Once the data parsing format is changed, which function renders the data? |
|
@AghaSaad04 Don't know, but you should be able to watch the events, or network calls, to find that out? Each importer also has an Update Preview button. No rush on this, you are doing great! |
There was a problem hiding this comment.
Sorry @AghaSaad04 for the delay in reviewing this.
I think this is a great PR which should be massively useful to users - thank you for that!
As mentioned above, I don't think this deserves to have its own checkbox in such a prominent location, both in the preview and grid mode. I think this display mode should be enabled by default and could be disabled in the preferences.
In terms of architecture, it would also be better to insert the unprintable character rendering code in the existing cell rendering code (for the grid UI, for the preview, facet values, expression preview…) rather than fixing the DOM after rendering using jQuery selectors (for instance to make a migration to a reactive framework easier).
|
I think prominent is important on this, it has affected many users. But I guess what Antonin is saying is because of that, let's just make it default on and hide the toggle in our preferences. I suspect we'll see later after release if users want to have that toggle exposed in the UI or not. (my hunch still says yes, but we'll see what the majority of users want and let them vote and speak for themselves, we trust them) |
|
I'm in favour with this being the default view and controlled in preferences (as described by @wetneb) If we want to have a place inside the project to control the view, the All->View menu would seem like the most appropriate place right now - but I prefer the idea of it being set in preferences as I think it's likely to be something people want to set across all projects |
|
So I just googled a bit about this problem, this is a bug in Firefox which has been reported in bug_zilla for more than 10 years. There are workarounds for this problem, will implement them. |
|
There are some libraries I tried, unfortunately, they are giving the same issues in firefox :( |
|
Would this be an easier way for now? https://lonekorean.github.io/highlight-within-textarea/ I basically see a lot of options when doing this kind of Google search: how to highlight text inside a textarea Did you try this one? https://markjs.io/ |
The issue with this approach is that this plugin will not only highlight the control characters but also those words having control characters in them: Examples: and yeah googling for alternatives. |
|
yes, and couldn't you "adapt" that code to work with ? or ask Will how one would go about that? perhaps opening an issue with his library to support that functionality? |
There was a problem hiding this comment.
Thanks! This issue is pretty involved, congrats for your tenacity.
Importing Thad's test file, I get this display with Chromium:
In Firefox, this renders correctly, but I have another problem:
It is impossible to add characters after the BEL, or to erase the BEL character itself (which should be a pretty common use case, I think). More precisely, if I click to the right of BEL, I do not get a cursor there (as if the field was not editable).
|
@wetneb, thanks for reviewing. For Firefox:It's a bug in firefox you cannot add after the contenteditable="false" element if it is at the end of the text. Also, you cannot delete the contenteditable="false". For chromium I will review again. |
|
So I just reviewed the Chromium issue, and got to know the localStorage function I am using is not supported by Chromium :/ |
|
I would not worry about caching the result of a command in local storage - you could run into issues of the cache being out sync with the preferences, and you can generally assume such a command is very quick, since the server is generally running locally. If we want to minimize the number of queries (which could be very useful for hosted versions), we could refactor all calls fetching preferences such that all preferences are fetched just once and stored in a global scope (without caching them in local storage). Anyway I think the problem you are tackling here is hard enough - don't make your life unnecessarily complicated with that sort of detail (that is a form of premature optimization!) |
AghaSaad04
force-pushed
the
non-printable-characters
branch
from
2f76008
to
7bceff0
Compare
AghaSaad04
force-pushed
the
non-printable-characters
branch
from
fbf1563
to
c2b85ba
Compare
Signed-off-by: Agha Saad Fraz <agha.saad04@gmail.com>
| @@ -777,3 +785,162 @@ function tags_to_control(str) { | |||
| } | |||
| return str; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Wow, that's a serious amount of custom code, just for FF support! Maybe this is a code smell that we are doing something wrong in our approach? Don't know, not an expert.
There was a problem hiding this comment.
Not sure though. As there are some plugins I tried them as well those were not working on FF due to this backspace issue.
| @@ -37,6 +37,8 @@ Refine.PreviewTable = function(projectData, elmt) { | |||
| this._render(); | |||
| }; | |||
|
|
|||
| var controlCharacters = ["NUL", "SOH", "STX", "ETX", "EOT", "ENQ", "ACK", "BEL", "BS", "TAB", "LF", "VT", "FF", "CR", "SO", "SI", "DLE", "DC1", "DC2", "DC3", "DC4", "NAK", "SYN", "ETB", "CAN", "EM", "SUB", "ESC", "FS", "GS", "RS", "US", "SPACE"]; | |||
There was a problem hiding this comment.
Let's replace SPACE with just SP to reduce verbosity
There was a problem hiding this comment.
yeah, was thinking that as well. I will replace that now.
| var charCode = cell.v.charAt(character).charCodeAt(0); | ||
| if (charCode <= 32) { | ||
| var size = (controlCharacters[charCode].length + 2) * 2; | ||
| unprintableChar = "<input type='button' class='unprintableCharacters' disabled='disabled' width=" + size + "% value=" + controlCharacters[charCode] + ">"; |
There was a problem hiding this comment.
To me it looks much better if change unprintableCharacters to have a style="padding-left: 2px;padding-right: 2px;" added. Otherwise it seems there's a default padding of 8 on both sides and makes it too stretched?
There was a problem hiding this comment.
Sure. I will update the styling.
| @@ -436,3 +436,10 @@ p.body-text { | |||
| height: 100%; | |||
| overflow: hidden; | |||
| } | |||
|
|
|||
| .unprintableCharacters{ | |||
There was a problem hiding this comment.
Also, for some reason, I think the margin-top is getting accumulated? I preferred a setting of margin-top: -3px;
There was a problem hiding this comment.
Double check all that in both FF and Chrome.
|
This clearly represents a ton of work (and review), so I'm reluctant to close it, but there haven't been any updates in three months. If @AghaSaad04 isn't available to push it over the finish line, is it close enough that someone else could pick it up? (I haven't looked at it myself, so asking @thadguidry and @wetneb who seem to have done most of the review). |
|
@tfmorris As a user, I've been pretty happy with it, and have a build with it merged into 3.3 myself that I have been using on occasion to have this long-standing needed feature in OpenRefine. It's quite a useful thing actually and I have it defaulted on in preferences on that custom build. I haven't had any problems other than a tiny small alignment issue between Chrome and Firefox, but its so minor and I bet @antoine2711 could find the issue on that sometime later after merge (or before if he can). Anyways, that's my take on testing and using it. Have no idea if it's performance bottle-necking for @lisa761 's ongoing work and didn't check all that, only against 3.3 tag and merge/build. So I'd say it definitely needs more testing against master here. |
|
Yes this is impressive work. On my side I am not sure we can pull this confidently, because of the complexity of the code to support editing. Perhaps one way forward would be to highlight non-printable characters only in non-editable locations first, and leave the support in form inputs for a follow-up PR. |
|
@wetneb I agree with the way forward on this. Now the question is who can make that happen? |
|
I am now wondering... For rendering in the Expression Editor: For rendering in the data grid: (Syntax Highlighting in ACE is actually done with a common standard now in place that nearly all Javascript code editors use and developed by PCWalton who worked with Mozilla Labs and Rust) |
|
I am going to close this as stale, given that there has not been any activity for many months. I think what went wrong here is that @thadguidry did a lot of end-user reviewing upfront, but developers did not have time to review the PR code-wise. That pushed @AghaSaad04 to make a lot of changes, investing a lot of time in the PR to match the end user requirements, but building up a bit of a house of cards on the code side. So:
If anyone wants to take that forward it is of course possible to open a new PR, possibly from the same branch. |
|
@wetneb ??? No pushing here. It was a collaboration. I do agree that within this @AghaSaad04 and I were both trying to see the limits of what could be done, that's all. In reality it was an EPIC, as so many of our single issues are actually, but we don't spend the time to break them up that way. That is something we should all try to improve on. My opinion was that this was a terrific collaboration that met some technical hurdles for @AghaSaad04 with our current UI framework and the interactions of him coming to grips with some of it's pain points, along with learning new things along the way. I definitely commend @AghaSaad04 on actually being successful in showing the non-printable chars in the Expression Editor...which was the primary pain point for users. Since we give users other means to determine if there are non-printable (control chars) in the data grid like with the Unicode facet. I think going forward for anyone on this PR and its issue, is that indeed they focus on 1 area at a time to display the non-printable chars, which @AghaSaad04 did get working quite well on that branch. |
|
@thadguidry of course that was a collaboration: my point is that by bringing up a lot of small details in your reviews (which is very useful in general), you are encouraging them to invest more and more time into the PR. As a team, it is our responsibility to ensure that we only do that when we are confident that the PR can make it, otherwise it can be quite disappointing for contributors. It's really the developers' fault here - we should have chimed in earlier on. But in these cases it would be good if you could slow down a tiny bit perhaps and remind us that we need to have a look at it? |
Signed-off-by: Agha Saad Fraz agha.saad04@gmail.com
I have added a feature of displaying non-printable characters. It is a fix for #1286.
Steps:
Demo:
Data grid without non-printable characters:
Data grid with non-printable characters: