Desktop: Add word counter feature to notes #2444
Conversation
|
Can you maybe add a screenshot or describe what this is doing? Did you create a status bar at the bottom or add another dialog to retrieve that info?
|
|
@tessus Hopefully the followup comments with links were helpful. I did not intend to create that initial comment, but there's no going back now. |
|
You can always edit the comment. But even with the links it's kind of awkward. A description should describe the PR. Otherwse it's like: I did something, test it, figure it out yourself. |
jdrobertso
changed the title
|
Pre-commit hook needs to be installed: https://travis-ci.org/laurent22/joplin/jobs/646244420#L4014 Also ideally new components should be developed as .tsx files as stated in CONTRIBUTING.md https://github.com/laurent22/joplin/blob/master/CONTRIBUTING.md#coding-style |
|
I am having issues with this build that I don't quite understand (I'm fairly new to React so that may be the problem). I have my files built in a way that seems correct according to how I understand react hooks to work, but for some reason when trying to run this code I get a build that passes, and then a fatal error on loading (screenshot below).
As far as I can tell, I'm not using import anywhere, though it is being used successfully in other places in the app, so I'm not sure what's causing this failure. I'm going to do more digging after work, but I was wondering if you had any insights. |
package.json
Outdated
| @@ -31,6 +31,7 @@ | |||
| "@types/react-dom": "^16.9.4", | |||
| "@typescript-eslint/eslint-plugin": "^2.10.0", | |||
| "@typescript-eslint/parser": "^2.10.0", | |||
| "countable": "^3.0.1", | |||
There was a problem hiding this comment.
That should be in normal dependencies, not devDependencies.
|
At what point you get this error? Could you provide the full context (what command was started, etc.)? Also you need to add Otherwise to fix this kind of error with no proper stack, the best is to comment out your changes, maybe even the entire file you've added and work from there. It might be some issue with one of the require statement. |
|
It was an issue with the require statement on the MainScreen file. I was still calling the |
|
I'm marking this ready to review until I have more information regarding the icon. This latest version does not include the 'Paragraphs' content either.
Additional screenshots:
|
tessus
added
reviewed-todo
desktop
| }; | ||
|
|
||
| const theme = themeStyle(props.theme); | ||
| const [textProperties] = useState<TextPropertiesMap>({}); |
There was a problem hiding this comment.
useState gives a getter and setter, and you should use the setter to update the text properties (see the React Hook doc for some examples).
| flexDirection: 'row', | ||
| alignItems: 'center', | ||
| }, | ||
| button: { |
There was a problem hiding this comment.
Please use the existing theme.button for the button style.
| const keyToLabel: KeyToLabelMap = { | ||
| words: _('Words'), | ||
| characters: _('Characters'), | ||
| characters_no_space: _('Characters excluding spaces'), |
There was a problem hiding this comment.
Thanks for catching that. My brain thinks in ruby sometimes, I missed it on my second check.
|
|
||
| const createItemField = (key: any, value: any) => { | ||
| const labelComp = <label style={Object.assign({}, theme.textStyle, { marginRight: '1em', width: '10em', display: 'inline-block', fontWeight: 'bold' })}>{keyToLabel[key]}</label>; | ||
| const controlComp = <div style={Object.assign({}, theme.textStyle, { display: 'inline-block' })}>{value}</div>; |
There was a problem hiding this comment.
Is it possible to reuse the style from the Info dialog for the label and control box?
| }; | ||
|
|
||
| const renderTextComponents = () => { | ||
| setTextComps(textProperties); |
There was a problem hiding this comment.
Shouldn't set state when rendering as it can create an infinite loop. Please use useEffect(() => { ... }, []) to set the text properties when the component mounts.
|
For the icon I'm not sure either. How about adding a menu item instead and removing the button? |
That is how it's done in Google Docs, and I was thinking the same might be a good fit here. It may take a little more work to figure out which document we're word counting, I'm not sure yet, but I'm sure it can be done. I was looking at the Android app (I don't have the mobile dev build set up yet, but I use it on my phone), and I think it would fit well in the dropdown menu there. It could be either on the 'Properties' pane or as another option on that same dropdown with a similar pane. I'll get to work on implementing the feedback and should have some changes pushed up tonight or Sunday. Thanks! |
|
@jdrobertso one small request: could you add another metric? Number of lines? For me the lines or more important than the characters and words. |
|
@tessus That shouldn't be a problem. The Ace Editor has a get length function that I think will give an accurate count of lines in the doc. I haven't tested it yet, but I'll report back when I get to it. Thanks for the feedback. |
|
I have run into a snag that I'm not sure of the best way to solve. The Ace Editor line functionality matches up with the Countable paragraph count. Basically, until there's a return character it is not considered a new line by the editor. However, I don't know whether to call that technically correct, as it is internally consistent with the 'view' content on the right hand side. Example screenshots below.
So, while I would not think of that as 7 'lines' if I were writing longer-form paragraph-based content, the viewer displays that as 7 lines and the editor thinks of it as 7 lines. I could, potentially, write a function that takes the display width of the editor and splits the lines up based on how much of that width an individual character takes, but before I go to that effort I wanted to check in here. Any feedback on this @tessus? I still have more work to do on switching from a toolbar button to a dropdown button (I'm thinking Ctrl+Shift+C for a shortcut, if that seems agreeable), but I wasn't sure how to proceed regarding lines. |
|
I'm a bit confused. First you say that Making up a new definition for a line is wrong. However, if you insist on doing this, how about creating your own metric (e.g. |
|
As long as your definition and the editor's definition agree, I'm good with it. I just wasn't sure what the expectation was regarding word wrap. I'll go with that definition and continue working on the other requested changes. Thanks. |
ElectronClient/app/gui/NoteText.jsx
Outdated
| text: this.rawEditor().session.getValue(), | ||
| text: this.getSessionText(), | ||
| lines: this.getSessionLines(), | ||
| }); | ||
| }, | ||
| }); | ||
|
|
||
| return toolbarItems; | ||
| } | ||
|
|
||
| getSessionText() { | ||
| return this.rawEditor().session.getValue(); | ||
| } | ||
|
|
||
| getSessionLines() { | ||
| return this.rawEditor().session.getLength(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Could you use this.state.note.body to get the text? Then lines = body.split("\n").length. This is because using rawEditor means another hard-dependency to Ace Editor, while we'd like to move away from it, and normally you can have all the required info from this.state.note.
I don't see a line count really making sense in a markdown editor (nor with an HTML editor). Only in the rendered text does a line become quantifiable (and it changes with the window size). In the editor, a newline doesn't have much meaning unless it is preceded by two spaces via an html Anyway. I would stick with a word count and a character count. If you want to do a line count, you can, but I would make a point of indicating whether it's lines as show in the rendering or in the editor (or both?)and certainly not base it off of newlines. And have the linecount change with the window size of course. Or just skip it. Or just live with the imperfection of whatever tool is being used. My 2-cents. |
|
I would say if the line count is controversial, let's leave out of the PR for now. It's a nearly complete PR so let's try to get it merged. |
|
@laurent22 I'm sorry but I don't think it's controversal. The same way the word count could be controversal. Why do I need a word count? Words are not the same lengtth, so why do we need it? |
|
@laurent22 Sorry this one took so long. I went ahead and added in the line count, I think @tessus is correct about the use case for this particular functionality and it's easy to ignore extra information if you don't need it, and harder to create information if you don't have it. Please let me know if you notice anything I didn't address from your feedback. |
|
The conflicts will have to be resolved before we can merge. (Please backup your local branch. A few people destroyed their code while resolving conflicts lately.) |
|
Working on it now, should be done shortly. |
|
Thanks, Laurent changed the build process (which also entailed getting rid of the app dir), thus some of the PRs have conflicting files. |
Fix errant whitespace changes
Use React hooks remove extra theme set Update styling function
I meant in the sense that there was different opinions about this in this thread and if it blocks the PR it's ok to leave it aside for now. But if you reached a consensus then of course go ahead and leave the feature in, I agree it's best with it than without it. |
|
I think this latest build should fix the issues the last one was having. I also opted to leave the counter as a button on the screen, rather than adding it to one of the menus. I was having some issues with figuring out the best way to get the text of the correct note, and figured this way at least gets a start out there that can be improved upon as feedback comes in. |
This makes sense of course. I misunderstood. |
|
Perfect! Thanks @jdrobertso for the pull request, and thanks @tessus for checking and testing. |
|
@jdrobertso, I've noticed a small bug in the line count: |
|
@laurent22 Just to be clear, we want this logic to apply only to an empty note, not to all empty lines, correct? My understanding was that 1 line, even a blank line, is still a line. However, if that's not expected behavior I can definitely implement a fix. The logic is pretty simple there. |
Again, this is a markdown editor and not a text editor even though some use it as such (hard line breaks and don't use the renderer). So, it boils down to: What does a "line" mean to a markdown editor? I'm unsure how to answer that (but I will attempt below). For that matter, what does a word count mean when you can have a bunch of html tags and such (non-words)? I don't think this feature needs to be smarter than it needs to be (word count can be + or - some acceptable range, but ... This feature is far more complex than people realize, I think. In an ideal world (and maybe I will file an RFE just to have it filed), I would make the feature offer two columns of numbers (I have seen at least one other editor do it this way): 1) for the editor, and 2) for the renderer. The number of lines in the editor would be the raw lines (like a text editor) and word count and character count would arguably include stuff inside markup. The column of numbers for the rendered content would only include what is visible (metadata and "code" would not longer be part of the word or character count. The line count would change based on number of lines rendered due to word-wrap. A blank line in both cases would count as a line. I believe anyway. I think that would be the "right way". IMHO. ... Finally. An empty note doesn't have a blank line. It has an end of file marker. Or should. That's not a line. Finally finally: @jdrobertso Thank you so much for this feature. Among the many hats I wear, I'm also a writer. Word count is a fundamental metric to all writers. This addresses an important gap in the application. Don't let our bellyaching over the nuance of the feature make you think we don't appreciate the work you did, because we do. Thank you. |
|
I agree I don't think line count in a non-code editor is very useful, but it's not too hard to add so why not. Bear has paragraph count which perhaps would be more useful. |
|
@taw00 I was a writer in my past life (prior to becoming a software engineer) and I agree that word count is fundamental. I never really used Joplin as a daily driver, I mostly use it for taking notes and jotting down some markdown, but I can see where a word count feature would be handy to have if I were still writing articles for a newspaper or something similar, so it made sense to put it in. It did turn out to be a little more nuanced than I was expecting, and required a little extra thought on my part (which I'm not averse to), but it was fun to do and to contribute to a tool I use myself. Thanks for the opportunity. |
Related to: #160
Moving the discussion from this question to this pull request.
There is an open request to add a character/word count feature to the content of the notes.
The additional button on the far right of the bar opens a dialog box that contains the content in the second screenshot below.
I am currently unsure of the best icon to use for this button, and the 'paragraphs' functionality of the Countable.JS library does not provide the best output in this case. Before submitting for review, I think I'm going to remove that functionality unless there are objections.