New StyledEdit features: word count & show document as read-only

[negusnyul]

The attached patch adds two new features to StyledEdit:

• Word count: a new item in the Document menu, it shows the number of lines, characters and words of the current file.

• Show as read only: a new checkable item in the Document menu: if checked, all the editing functions are disabled. (However, the user still can save the document - this is NOT the 'read-only' file attribute, so the file won't be read-only, but I think this feature is useful when one doesn't want to modify opened files accidentally.)

I read & applied the coding style guide, so I think the code style is OK, and the patch adds very few lines of code, but if anything is wrong with it, please notify me.

(Note: This is my first patch, I did this when I first checked out the code from SVN and played around a bit with it.)

[humdinger]

Hey negusnyul! Welcome!
I'm not really sure about the "read-only" feature. At least I have never felt the need for it. If I don't want to save an accidentally altered document I just click "Don't save" in the alert popping up when closing the document. Maybe others have a different opinion. If so, I'd suggest to rename the menu item to "Lock" or something to avoid confusion with actual read-only-ness.

The word count might be handy. If you feel like it, you could rename it to "Info" and add a few more essentials. See Pe's "Statistics" in "Window|File Options..." for inspiration. One could also take the currently selected text into account to show something like Lines: 124 of 1928 .

For the coding style I defer to the local coding style police :)
All I can say is that the use of the curly brackets in if/for/switch-case blocks should be re-read in the ​Guidelines. The opening bracket should be on the same line, if needed at all. Also, respect those 80 chars/line.

In my quite unexperienced opinion, it's a rather fine first patch, though it's always a good idea to discuss a planned feature addition beforehand. We have to be wary against feature-creep and a fix of something is always less controversial.

[axeld]

I'm also not sure about the "read-only" feature; since you have to enable it manually, it has only very limited use. One could set it automatically depending on the actual POSIX permissions of the file, though. OTOH I am always annoyed by editors that do so (like Pe), as it doesn't have any practical use other than to notify you that you may not have the rights to edit the file at the current location.

Furthermore, if you accidentally change a file (how often does this happen, anyway?), no one forces you to save it, and there is always the possibility to undo your last action.

About the coding style, one thing I have to add to humdinger: use '\t' when you need a tab character constant, don't just use an actual tab, as that cannot be seen. In this particular case, you may want to use isspace() instead, though, anyway, or rather BUnicodeChar::IsSpace().

[negusnyul]

Hi! Thank you for the fast responses! I decided to remove the 'Show as read only' feature. I fixed the coding style issues, and now the code uses BUnicodeChar::IsSpace()

(I attached the new patch.)

And next time when I write a new feature I will ask before :) (btw, is the IRC channel a good place for that?)

[axeld]

Thanks, and I've got some more remarks for you:

• your patch also contains some unrelated changes. Is there any particular reason you introduce new members for the menu items? Since they are never used after construction, there is no need to have an extra member for them (neither for the word count menu item). In any case, you should always try to make your patches as focussed as possible; better make two patches for unrelated changes.
• You also introduce a bug with the member changes, at least line 224 still points to the old name when calling SetTarget() (which should break paste functionality).
• Since you also count lines, and characters "word count" is actually not the best name "Statistics" or something like it would be better. Also, a method name that actually implies that it also shows an alert would be preferred, for example ShowStatistics().

Anyway, thanks for working on this, and I hope you don't get shied away easily :-)

[negusnyul]

Hi!

About the unrelated changes:

That changes were needed by the show-as-read-only feature. I thought it is better not to remove them. (Maybe some future use?) But now, I'll remove them, not a big work to reintroduce them if needed.

"You also introduce a bug with the member changes, at least line 224"

:-O

Thats really-really embarrassing. Fixed. I also fixed the mentioned names.

(Third version of the patch attached.)

"Anyway, thanks for working on this, and I hope you don't get shied away easily :-)"

Its not that easy to shy me away :) Also, I would like to see this patch in trunk sooner or later, so I keep working on it if needed. :)

[axeld]

Replying to negusnyul:

(Third version of the patch attached.)

Great! I've got only another remark that I missed before (sorry!): you've used uint64 for the size of the strings - that is a bit more than what can the put into memory, and both BString::Length() (that you used), as well as BTextView::TextLength() (that you should have used) return an int32 (which probably should have been a size_t), which should therefore be enough for length, and the index. Since it's only a small change, I could fix this for you, too, if you prefer.

"Anyway, thanks for working on this, and I hope you don't get shied away easily :-)" Its not that easy to shy me away :) Also, I would like to see this patch in trunk sooner or later, so I keep working on it if needed. :)

Is that a promise? Because we could keep this patch from being applied until StyledEdit is a full featured word processor, if you like to ;-)

[negusnyul]

Fourth (and hopefully final) version attached. I replaced uint64 to size_t and BString::Length() to BTextView::TextLength()

About my future plans:

1. I will try to fix some bugs releated to the applications (and other high-level components) shipped with Haiku

2. Implement some improvements to the existing applications (possibly more StyledEdit features, too)

3. I can imagine a TODO list application or desktop applet, a calendar, a dictionary, etc. This will need more discussion, and more experience from my side :) (I have 2,5+ years C++ experience, but never used or programmed BeOS/Haiku before.)

4. I want to take a look at the kernel, drivers and other low-level parts because I have no experience in that at all and I think it's an interesting topic. (But this will be in the far future.)

So, please apply this patch if you think it's ready for that.

[axeld]

Thanks for the updated patch! Don't worry, I wasn't serious - I've applied it in hrev39546 :-)

I've applied a style change to StyledEdit in hrev39549 - as you probably noticed, StyledEdit wasn't following our style guide too well. It's still not perfect, but I improved the situation a bit. Looking forward to your future patches!