Review Request 129324: VCSCommitDiffPatchSource point changes : set width to 72 characters and use deleteLater()

Discussion:

René J.V. Bertin

2016-11-03 17:32:12 UTC

Permalink

Thanks,

RenÃ© J.V. Bertin

Aleix Pol Gonzalez

2016-11-04 00:40:51 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129324/#review100548
-----------------------------------------------------------

vcs/widgets/vcsdiffpatchsources.cpp (line 59)
<https://git.reviewboard.kde.org/r/129324/#comment67501>

Shouldn't it be the preferred width?

vcs/widgets/vcsdiffpatchsources.cpp (line 218)
<https://git.reviewboard.kde.org/r/129324/#comment67503>

Shouldn't it be `m_commitMessageWidget`?

vcs/widgets/vcsdiffpatchsources.cpp (line 219)
<https://git.reviewboard.kde.org/r/129324/#comment67502>

Not that it's wrong, but is it possible to have a backtrace?

- Aleix Pol Gonzalez

Post by RenÃ© J.V. Bertin
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/129324/
-----------------------------------------------------------
(Updated Nov. 3, 2016, 6:32 p.m.)
Review request for KDE Software on Mac OS X and KDevelop.
Repository: kdevplatform
Description
-------
This patch proposes 2 point changes to `VCSCommitDiffPatchSource`. They're unrelated but both so small and limited to a single file that I hope you won't mind I put them in a single RR.
1- KDE insists on a 72 character limit for commit messages, and that is probably a very reasonable standard. In the current implementation there is no easy way to judge how far from that threshold we are. The easiest way to do this is by setting the commit message editor to a width of 72 characters (which for some reason requires taking the width of a string of 74 characters). I think there is room enough for this, but a more complete implementation would probably introduce a resize handle between the 2 main widgets. I haven't yet tried to look into that.
2- The commit message has been a very likely source of the crashes I've seen in the past (on Mac) when closing patch reviews, and which have been addressed by using `QObject::deleteLater()` for select widgets. That change was somehow never submitted for the commit message editor.
Diffs
-----
vcs/widgets/vcsdiffpatchsources.cpp 42cf1d8
Diff: https://git.reviewboard.kde.org/r/129324/diff/
Testing
-------
On OS X with 2 different fixed font typefaces (sizes). The deleteLater() change has been in use since about 4.7.1 .
File Attachments
----------------
preview: 72-char wide commit message editor
https://git.reviewboard.kde.org/media/uploaded/files/2016/11/03/2ffdd73a-db19-4bf2-9545-7a850a4b7fd1__Screen_Shot_2016-11-03_at_18.24.54.png
Thanks,
RenÃ© J.V. Bertin

René J.V. Bertin

2016-11-04 01:09:43 UTC

Permalink

Post by RenÃ© J.V. Bertin

vcs/widgets/vcsdiffpatchsources.cpp, line 218
<https://git.reviewboard.kde.org/r/129324/diff/1/?file=483956#file483956line218>
Shouldn't it be `m_commitMessageWidget`?

Oops, yes. I don't now what went wrong here, but now that you draw my attention to it, is the check actually necessary? Either `data()` call returns the result of a `new` operation, so should never be NULL under normal conditions, right?

Post by RenÃ© J.V. Bertin

vcs/widgets/vcsdiffpatchsources.cpp, line 219
<https://git.reviewboard.kde.org/r/129324/diff/1/?file=483956#file483956line219>
Not that it's wrong, but is it possible to have a backtrace?

I'll try to generate one.

Post by RenÃ© J.V. Bertin

vcs/widgets/vcsdiffpatchsources.cpp, line 59
<https://git.reviewboard.kde.org/r/129324/diff/1/?file=483956#file483956line59>
Shouldn't it be the preferred width?

Aleix Pol Gonzalez

2016-11-04 01:44:44 UTC

Permalink

Post by RenÃ© J.V. Bertin

vcs/widgets/vcsdiffpatchsources.cpp, line 59
<https://git.reviewboard.kde.org/r/129324/diff/1/?file=483956#file483956line59>
Shouldn't it be the preferred width?

Here it's explained: http://doc.qt.io/qt-4.8/qwidget.html#size-hints-and-size-policies

I wouldn't want the window to get stuck, it's especially annoying when the window doesn't fit the screen, I can see it happening if the user has a relatively big font. But, maybe it's best that we test and act if it's problematic.

- Aleix

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129324/#review100548
-----------------------------------------------------------

René J.V. Bertin

2016-11-04 08:21:43 UTC

Permalink

Post by RenÃ© J.V. Bertin

vcs/widgets/vcsdiffpatchsources.cpp, line 59
<https://git.reviewboard.kde.org/r/129324/diff/1/?file=483956#file483956line59>
Shouldn't it be the preferred width?

Sorry, it was around 2am when I replied, so I only thought but forgot to add something like "in practice, beyond what the docs say". Is the 4.8 documentation better in this aspect, btw?

I'll see if the parent widget width information is available (reliable) at this point so we can check the target widget against that.

FWIW, I tried using `contentMargins()` and a 72-character string to get the desired width, but apparently there are other margins I don't know how to obtain which make we need a 74 character string to get 72 visible characters. Doesn't really inspire a lot of trust in precise layout'ing with high-level methods ;)

How hard would it be to add something like a red line inside the commit message editor, at the 73rd column? That way we could use the preferred size and be done with it. I'm pretty sure I've seen such a feature in a KDE app but cannot remember at all where.

- RenÃ© J.V.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129324/#review100548
-----------------------------------------------------------

Aleix Pol Gonzalez

2016-11-04 11:21:23 UTC

Permalink

Post by RenÃ© J.V. Bertin

vcs/widgets/vcsdiffpatchsources.cpp, line 59
<https://git.reviewboard.kde.org/r/129324/diff/1/?file=483956#file483956line59>
Shouldn't it be the preferred width?

Exactly how does that change the likelihood that the widget really gets to be 72 characters wide?
I'd think that this kind of extremely simple approach only works when the width is set reliably. Otherwise we'd need some additional indicator if we want to provide a line-width reference. (And that might quickly require more effort than justified?)
Here it's explained: http://doc.qt.io/qt-4.8/qwidget.html#size-hints-and-size-policies
I wouldn't want the window to get stuck, it's especially annoying when the window doesn't fit the screen, I can see it happening if the user has a relatively big font. But, maybe it's best that we test and act if it's problematic.
Sorry, it was around 2am when I replied, so I only thought but forgot to add something like "in practice, beyond what the docs say". Is the 4.8 documentation better in this aspect, btw?
I'll see if the parent widget width information is available (reliable) at this point so we can check the target widget against that.
FWIW, I tried using `contentMargins()` and a 72-character string to get the desired width, but apparently there are other margins I don't know how to obtain which make we need a 74 character string to get 72 visible characters. Doesn't really inspire a lot of trust in precise layout'ing with high-level methods ;)
How hard would it be to add something like a red line inside the commit message editor, at the 73rd column? That way we could use the preferred size and be done with it. I'm pretty sure I've seen such a feature in a KDE app but cannot remember at all where.
How hard would it be to add something like a red line inside the commit message editor, at the 73rd column? That way we could use the preferred size and be done with it. I'm pretty sure I've seen such a feature in a KDE app but cannot remember at all where.

We already have this: Loading Image...

- Aleix

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129324/#review100548
-----------------------------------------------------------

René J.V. Bertin

2016-11-04 13:37:23 UTC

Permalink

Post by RenÃ© J.V. Bertin

vcs/widgets/vcsdiffpatchsources.cpp, line 59
<https://git.reviewboard.kde.org/r/129324/diff/1/?file=483956#file483956line59>
Shouldn't it be the preferred width?

How hard would it be to add something like a red line inside the commit message editor, at the 73rd column? That way we could use the preferred size and be done with it. I'm pretty sure I've seen such a feature in a KDE app but cannot remember at all where.

We already have this: http://i.imgur.com/nyDOcrU.png

Ah! I never noticed that, in fact I haven't been able to duplicate what you're showing in the linked image. Seems spell-checking perturbs the feature, and I'm not getting the tooltip either.
Come to think of it, I've always wondered what kind of glitches caused the spellchecker to flag correctly spelled words and in changing colours.

What I was thinking of is a vertical red line at the 72nd or 73rd column, spanning the full visual height. I suppose the existing QTextFormat method could also be used to change the text colour (for `!warning`), but then the problem is to find a colour that keeps the text readable for all background colours, right?

- RenÃ© J.V.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129324/#review100548
-----------------------------------------------------------

Aleix Pol Gonzalez

2016-11-04 14:16:51 UTC

Permalink

Post by RenÃ© J.V. Bertin

vcs/widgets/vcsdiffpatchsources.cpp, line 59
<https://git.reviewboard.kde.org/r/129324/diff/1/?file=483956#file483956line59>
Shouldn't it be the preferred width?

I was thinking that maybe it would make sense to have a full-fledged kate view there and get all the features at once. It definitely supports having a vertical line and all of the highlighting.

- Aleix

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129324/#review100548
-----------------------------------------------------------

René J.V. Bertin

2016-11-04 14:50:24 UTC

Permalink

Post by RenÃ© J.V. Bertin

vcs/widgets/vcsdiffpatchsources.cpp, line 59
<https://git.reviewboard.kde.org/r/129324/diff/1/?file=483956#file483956line59>
Shouldn't it be the preferred width?

Is that just a matter of replacing `KTextEdit` with however a kate view is called internally, or something "a little" more complicated than that?

You're not wrong, but in the end the editor serves to compose a "shortish" commit message which won't have any formatting other than line length and potentially a few empty lines at the appropriate place. After all nothing stands in the way of opening a new temp. document to compose a more elaborate message (and that view should rarely be too small and require resizing). True, you won't get a 72-character (or 65) width limit, but there's a line and column counter right where you'd expect it.

If you do want to open a dedicated Kate-based view: is it possible to split the Overview window horizontally, and use the bottom portion as an *additional* commit message editor (and take the message from there if the standard editor is empty)? If so that feature could be put under a button.

- RenÃ© J.V.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129324/#review100548
-----------------------------------------------------------

René J.V. Bertin

2016-11-04 16:57:57 UTC

Permalink

Post by RenÃ© J.V. Bertin

vcs/widgets/vcsdiffpatchsources.cpp, line 59
<https://git.reviewboard.kde.org/r/129324/diff/1/?file=483956#file483956line59>
Shouldn't it be the preferred width?

We already have this: http://i.imgur.com/nyDOcrU.png
Ah! I never noticed that, in fact I haven't been able to duplicate what you're showing in the linked image. Seems spell-checking perturbs the feature, and I'm not getting the tooltip either.
Come to think of it, I've always wondered what kind of glitches caused the spellchecker to flag correctly spelled words and in changing colours.
What I was thinking of is a vertical red line at the 72nd or 73rd column, spanning the full visual height. I suppose the existing QTextFormat method could also be used to change the text colour (for `!warning`), but then the problem is to find a colour that keeps the text readable for all background colours, right?
it seems that if I want to try the preferred size approach I'll have to subclass KTextEdit to give it a custom `sizeHint()` (or `minimumSizeHint()`) method, is that correct?
I was thinking that maybe it would make sense to have a full-fledged kate view there and get all the features at once. It definitely supports having a vertical line and all of the highlighting.
Is that just a matter of replacing `KTextEdit` with however a kate view is called internally, or something "a little" more complicated than that?
You're not wrong, but in the end the editor serves to compose a "shortish" commit message which won't have any formatting other than line length and potentially a few empty lines at the appropriate place. After all nothing stands in the way of opening a new temp. document to compose a more elaborate message (and that view should rarely be too small and require resizing). True, you won't get a 72-character (or 65) width limit, but there's a line and column counter right where you'd expect it.
If you do want to open a dedicated Kate-based view: is it possible to split the Overview window horizontally, and use the bottom portion as an *additional* commit message editor (and take the message from there if the standard editor is empty)? If so that feature could be put under a button.

In the meantime I've implemented a preferred minimum size from what I understand of the size policy. If I resize my main window to something very small, starting a (git) commit action increases the window size. I've never tried this kind of thing before, so maybe it's simply due to the overal widget width (the widget with the list of modified files is not created in `vcsdiffpatchsources.cpp`). In any case I don't see the window can be enlarged because the widget incidates it can be made narrower than the indicated minimum size.

- RenÃ© J.V.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129324/#review100548
-----------------------------------------------------------

René J.V. Bertin

2016-11-04 17:07:03 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129324/
-----------------------------------------------------------

(Updated Nov. 4, 2016, 6:07 p.m.)

Review request for KDE Software on Mac OS X and KDevelop.

Changes
-------

Implements most of the suggestions on the original patch, except a migration to a full Kate view.

I've also included some proposed changes to the git highlighter:
- it sets a tooltip summarising the commit message guidelines
- it avoids creating confusion with spell checker indications by no longer using underlining. Instead:
â¢ summary text exceeding the soft limit is set in italic font (and this applies no longer to the full line). IIRC this is a rather classical use of italic to signal an issue.
â¢ text exceeding the hard limit is ~~overlined~~.

I considered using bold face for hard limit exceeding, but bold is already used (for the summary) and apparently it depends on the typeface used whether the hint is respected at all (it's not for the fonts I've tried).

Repository: kdevplatform

Description
-------

This patch proposes 2 point changes to `VCSCommitDiffPatchSource`. They're unrelated but both so small and limited to a single file that I hope you won't mind I put them in a single RR.

In order of appearance:

1- KDE insists on a 72 character limit for commit messages, and that is probably a very reasonable standard. In the current implementation there is no easy way to judge how far from that threshold we are. The easiest way to do this is by setting the commit message editor to a width of 72 characters (which for some reason requires taking the width of a string of 74 characters). I think there is room enough for this, but a more complete implementation would probably introduce a resize handle between the 2 main widgets. I haven't yet tried to look into that.

2- The commit message has been a very likely source of the crashes I've seen in the past (on Mac) when closing patch reviews, and which have been addressed by using `QObject::deleteLater()` for select widgets. That change was somehow never submitted for the commit message editor.

Diffs (updated)
-----

plugins/git/gitmessagehighlighter.cpp da7660d
vcs/widgets/vcsdiffpatchsources.cpp 42cf1d8

Diff: https://git.reviewboard.kde.org/r/129324/diff/

Testing
-------

On OS X with 2 different fixed font typefaces (sizes). The deleteLater() change has been in use since about 4.7.1 .

File Attachments
----------------

preview: 72-char wide commit message editor
https://git.reviewboard.kde.org/media/uploaded/files/2016/11/03/2ffdd73a-db19-4bf2-9545-7a850a4b7fd1__Screen_Shot_2016-11-03_at_18.24.54.png

Thanks,

RenÃ© J.V. Bertin

Aleix Pol Gonzalez

2016-11-08 19:48:21 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129324/#review100734
-----------------------------------------------------------

I just tested it. Overline looks _really_ odd. -1

plugins/git/gitmessagehighlighter.cpp (line 41)
<https://git.reviewboard.kde.org/r/129324/#comment67616>

-1

vcs/widgets/vcsdiffpatchsources.cpp (line 60)
<https://git.reviewboard.kde.org/r/129324/#comment67615>

Set an initial value, I guess it should be 0 on construction.

vcs/widgets/vcsdiffpatchsources.cpp (line 81)
<https://git.reviewboard.kde.org/r/129324/#comment67613>

What's the problem? Something else changes the tooltip?

vcs/widgets/vcsdiffpatchsources.cpp (line 239)
<https://git.reviewboard.kde.org/r/129324/#comment67614>

No commented code

- Aleix Pol Gonzalez

Post by RenÃ© J.V. Bertin
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/129324/
-----------------------------------------------------------
(Updated Nov. 4, 2016, 6:07 p.m.)
Review request for KDE Software on Mac OS X and KDevelop.
Repository: kdevplatform
Description
-------
This patch proposes 2 point changes to `VCSCommitDiffPatchSource`. They're unrelated but both so small and limited to a single file that I hope you won't mind I put them in a single RR.
1- KDE insists on a 72 character limit for commit messages, and that is probably a very reasonable standard. In the current implementation there is no easy way to judge how far from that threshold we are. The easiest way to do this is by setting the commit message editor to a width of 72 characters (which for some reason requires taking the width of a string of 74 characters). I think there is room enough for this, but a more complete implementation would probably introduce a resize handle between the 2 main widgets. I haven't yet tried to look into that.
2- The commit message has been a very likely source of the crashes I've seen in the past (on Mac) when closing patch reviews, and which have been addressed by using `QObject::deleteLater()` for select widgets. That change was somehow never submitted for the commit message editor.
Diffs
-----
plugins/git/gitmessagehighlighter.cpp da7660d
vcs/widgets/vcsdiffpatchsources.cpp 42cf1d8
Diff: https://git.reviewboard.kde.org/r/129324/diff/
Testing
-------
On OS X with 2 different fixed font typefaces (sizes). The deleteLater() change has been in use since about 4.7.1 .
File Attachments
----------------
preview: 72-char wide commit message editor
https://git.reviewboard.kde.org/media/uploaded/files/2016/11/03/2ffdd73a-db19-4bf2-9545-7a850a4b7fd1__Screen_Shot_2016-11-03_at_18.24.54.png
Thanks,
RenÃ© J.V. Bertin

René J.V. Bertin

2016-11-08 20:21:08 UTC

Permalink

Post by RenÃ© J.V. Bertin

plugins/git/gitmessagehighlighter.cpp, line 41
<https://git.reviewboard.kde.org/r/129324/diff/2/?file=484065#file484065line41>
-1

Well, the goal is to draw attention... :) I considered overstrike which would be the most logical choice if it didn't affect readability a bit too much (but I didn't try it yet).

What other options are there that cannot be mistaken for information from the spellchecker and that also don't cannot make you wonder if you just typed a space or an underscore?

Post by RenÃ© J.V. Bertin

vcs/widgets/vcsdiffpatchsources.cpp, line 81
<https://git.reviewboard.kde.org/r/129324/diff/2/?file=484066#file484066line81>
What's the problem? Something else changes the tooltip?

No. Or maybe yes; I simply never get to see it. I wrote that off to being able to find the exact spot over the text to trigger the tooltip but you're right, maybe something else unsets it.

vcs/widgets/vcsdiffpatchsources.cpp, line 239
<https://git.reviewboard.kde.org/r/129324/diff/2/?file=484066#file484066line239>
No commented code

Sorry, accident.

- RenÃ© J.V.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129324/#review100734
-----------------------------------------------------------

Post by RenÃ© J.V. Bertin
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/129324/
-----------------------------------------------------------
(Updated Nov. 4, 2016, 6:07 p.m.)
Review request for KDE Software on Mac OS X and KDevelop.
Repository: kdevplatform
Description
-------
This patch proposes 2 point changes to `VCSCommitDiffPatchSource`. They're unrelated but both so small and limited to a single file that I hope you won't mind I put them in a single RR.
1- KDE insists on a 72 character limit for commit messages, and that is probably a very reasonable standard. In the current implementation there is no easy way to judge how far from that threshold we are. The easiest way to do this is by setting the commit message editor to a width of 72 characters (which for some reason requires taking the width of a string of 74 characters). I think there is room enough for this, but a more complete implementation would probably introduce a resize handle between the 2 main widgets. I haven't yet tried to look into that.
2- The commit message has been a very likely source of the crashes I've seen in the past (on Mac) when closing patch reviews, and which have been addressed by using `QObject::deleteLater()` for select widgets. That change was somehow never submitted for the commit message editor.
Diffs
-----
plugins/git/gitmessagehighlighter.cpp da7660d
vcs/widgets/vcsdiffpatchsources.cpp 42cf1d8
Diff: https://git.reviewboard.kde.org/r/129324/diff/
Testing
-------
On OS X with 2 different fixed font typefaces (sizes). The deleteLater() change has been in use since about 4.7.1 .
File Attachments
----------------
preview: 72-char wide commit message editor
https://git.reviewboard.kde.org/media/uploaded/files/2016/11/03/2ffdd73a-db19-4bf2-9545-7a850a4b7fd1__Screen_Shot_2016-11-03_at_18.24.54.png
Thanks,
RenÃ© J.V. Bertin

Aleix Pol Gonzalez

2016-11-08 23:23:16 UTC

Permalink

plugins/git/gitmessagehighlighter.cpp, line 41
<https://git.reviewboard.kde.org/r/129324/diff/2/?file=484065#file484065line41>
-1

Well, the goal is to draw attention... :) I considered overstrike which would be the most logical choice if it didn't affect readability a bit too much (but I didn't try it yet).
What other options are there that cannot be mistaken for information from the spellchecker and that also don't cannot make you wonder if you just typed a space or an underscore?

I tested it, my brain was saying it was an underline on the line above.
Maybe strike would work, or changing the font color.

- Aleix

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129324/#review100734
-----------------------------------------------------------

Post by RenÃ© J.V. Bertin
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/129324/
-----------------------------------------------------------
(Updated Nov. 4, 2016, 6:07 p.m.)
Review request for KDE Software on Mac OS X and KDevelop.
Repository: kdevplatform
Description
-------
This patch proposes 2 point changes to `VCSCommitDiffPatchSource`. They're unrelated but both so small and limited to a single file that I hope you won't mind I put them in a single RR.
1- KDE insists on a 72 character limit for commit messages, and that is probably a very reasonable standard. In the current implementation there is no easy way to judge how far from that threshold we are. The easiest way to do this is by setting the commit message editor to a width of 72 characters (which for some reason requires taking the width of a string of 74 characters). I think there is room enough for this, but a more complete implementation would probably introduce a resize handle between the 2 main widgets. I haven't yet tried to look into that.
2- The commit message has been a very likely source of the crashes I've seen in the past (on Mac) when closing patch reviews, and which have been addressed by using `QObject::deleteLater()` for select widgets. That change was somehow never submitted for the commit message editor.
Diffs
-----
plugins/git/gitmessagehighlighter.cpp da7660d
vcs/widgets/vcsdiffpatchsources.cpp 42cf1d8
Diff: https://git.reviewboard.kde.org/r/129324/diff/
Testing
-------
On OS X with 2 different fixed font typefaces (sizes). The deleteLater() change has been in use since about 4.7.1 .
File Attachments
----------------
preview: 72-char wide commit message editor
https://git.reviewboard.kde.org/media/uploaded/files/2016/11/03/2ffdd73a-db19-4bf2-9545-7a850a4b7fd1__Screen_Shot_2016-11-03_at_18.24.54.png
Thanks,
RenÃ© J.V. Bertin

René J.V. Bertin

2016-11-09 09:55:30 UTC

Permalink

vcs/widgets/vcsdiffpatchsources.cpp, line 60
<https://git.reviewboard.kde.org/r/129324/diff/2/?file=484066#file484066line60>
Set an initial value, I guess it should be 0 on construction.

Yeah, better, even if m_minWidth is assigned a value unconditionally.

plugins/git/gitmessagehighlighter.cpp, line 41
<https://git.reviewboard.kde.org/r/129324/diff/2/?file=484065#file484065line41>
-1

Well, the goal is to draw attention... :) I considered overstrike which would be the most logical choice if it didn't affect readability a bit too much (but I didn't try it yet).
What other options are there that cannot be mistaken for information from the spellchecker and that also don't cannot make you wonder if you just typed a space or an underscore?
I tested it, my brain was saying it was an underline on the line above.
Maybe strike would work, or changing the font color.

I see what you mean, I guess that if underlining whitespace doesn't look like underscores with a given font, overlining it could well look like underlining the line above. Can't have both...

I'm trying something else with a property I didn't notice before. When you exceed the soft limit I increase letter spacing, which is a nice approach in that it is a signal that is directly related to what you're warning against. Italic is now used for exceeding the hard limit, in combination with the additional spacing.

- RenÃ© J.V.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129324/#review100734
-----------------------------------------------------------

Post by RenÃ© J.V. Bertin
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/129324/
-----------------------------------------------------------
(Updated Nov. 4, 2016, 6:07 p.m.)
Review request for KDE Software on Mac OS X and KDevelop.
Repository: kdevplatform
Description
-------
This patch proposes 2 point changes to `VCSCommitDiffPatchSource`. They're unrelated but both so small and limited to a single file that I hope you won't mind I put them in a single RR.
1- KDE insists on a 72 character limit for commit messages, and that is probably a very reasonable standard. In the current implementation there is no easy way to judge how far from that threshold we are. The easiest way to do this is by setting the commit message editor to a width of 72 characters (which for some reason requires taking the width of a string of 74 characters). I think there is room enough for this, but a more complete implementation would probably introduce a resize handle between the 2 main widgets. I haven't yet tried to look into that.
2- The commit message has been a very likely source of the crashes I've seen in the past (on Mac) when closing patch reviews, and which have been addressed by using `QObject::deleteLater()` for select widgets. That change was somehow never submitted for the commit message editor.
Diffs
-----
plugins/git/gitmessagehighlighter.cpp da7660d
vcs/widgets/vcsdiffpatchsources.cpp 42cf1d8
Diff: https://git.reviewboard.kde.org/r/129324/diff/
Testing
-------
On OS X with 2 different fixed font typefaces (sizes). The deleteLater() change has been in use since about 4.7.1 .
File Attachments
----------------
preview: 72-char wide commit message editor
https://git.reviewboard.kde.org/media/uploaded/files/2016/11/03/2ffdd73a-db19-4bf2-9545-7a850a4b7fd1__Screen_Shot_2016-11-03_at_18.24.54.png
Thanks,
RenÃ© J.V. Bertin

René J.V. Bertin

2017-03-22 21:35:18 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129324/
-----------------------------------------------------------

(Updated March 22, 2017, 10:35 p.m.)

Review request for KDE Software on Mac OS X and KDevelop.

Changes
-------

rebased, dropped the deleteLater() change.

Summary (updated)
-----------------

VCS commit message width feedback

Repository: kdevplatform

Description
-------

This patch proposes 2 point changes to `VCSCommitDiffPatchSource`. They're unrelated but both so small and limited to a single file that I hope you won't mind I put them in a single RR.

In order of appearance:

1- KDE insists on a 72 character limit for commit messages, and that is probably a very reasonable standard. In the current implementation there is no easy way to judge how far from that threshold we are. The easiest way to do this is by setting the commit message editor to a width of 72 characters (which for some reason requires taking the width of a string of 74 characters). I think there is room enough for this, but a more complete implementation would probably introduce a resize handle between the 2 main widgets. I haven't yet tried to look into that.

2- The commit message has been a very likely source of the crashes I've seen in the past (on Mac) when closing patch reviews, and which have been addressed by using `QObject::deleteLater()` for select widgets. That change was somehow never submitted for the commit message editor.

Diffs (updated)
-----

plugins/git/gitmessagehighlighter.cpp da7660d3eebf9f836e49b48b41df2d11afe83268
vcs/widgets/vcsdiffpatchsources.cpp b080872d4366f65947244cd2c6299fbc14e9ea96

Diff: https://git.reviewboard.kde.org/r/129324/diff/

Testing
-------

On OS X with 2 different fixed font typefaces (sizes). The deleteLater() change has been in use since about 4.7.1 .

File Attachments
----------------

preview: 72-char wide commit message editor
https://git.reviewboard.kde.org/media/uploaded/files/2016/11/03/2ffdd73a-db19-4bf2-9545-7a850a4b7fd1__Screen_Shot_2016-11-03_at_18.24.54.png
commit message showing the overflow warnings/errors (taken on Linux)
Loading Image...

Friedrich W. H. Kossebau

2017-08-02 10:00:55 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129324/#review103528
-----------------------------------------------------------

RenÃ©, given this one has been transferred to phabricator, could you please help and also close it as discarded, to clean the list of request on reviewboard that still need to see some (final) care?

- Friedrich W. H. Kossebau

Post by RenÃ© J.V. Bertin
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/129324/
-----------------------------------------------------------
(Updated March 22, 2017, 9:35 p.m.)
Review request for KDE Software on Mac OS X and KDevelop.
Repository: kdevplatform
Description
-------
Transferred to https://phabricator.kde.org/D5139
This patch proposes changes to the way the VCS commit message editor provides feedback concerning the message width.
KDE insists on a 72 character limit for commit messages, and that is probably a very reasonable standard. In the current implementation there is no easy way to judge how far from that threshold we are. The easiest way to do this is by setting the commit message editor to a width of 72 characters (which for some reason requires taking the width of a string of 74 characters). I think there is room enough for this, but a more complete implementation would probably introduce a resize handle between the 2 main widgets. I haven't yet tried to look into that.
Several approaches have been tried during the course of this RR.
Diffs
-----
plugins/git/gitmessagehighlighter.cpp da7660d3eebf9f836e49b48b41df2d11afe83268
vcs/widgets/vcsdiffpatchsources.cpp b080872d4366f65947244cd2c6299fbc14e9ea96
Diff: https://git.reviewboard.kde.org/r/129324/diff/
Testing
-------
On OS X with 2 different fixed font typefaces (sizes). The deleteLater() change has been in use since about 4.7.1 .
File Attachments
----------------
preview: 72-char wide commit message editor
https://git.reviewboard.kde.org/media/uploaded/files/2016/11/03/2ffdd73a-db19-4bf2-9545-7a850a4b7fd1__Screen_Shot_2016-11-03_at_18.24.54.png
commit message showing the overflow warnings/errors (taken on Linux)
https://git.reviewboard.kde.org/media/uploaded/files/2016/11/09/54eb4035-0fe1-4d6c-8365-db48bf67b826__KDev-Commit-Dialog.png
Thanks,
RenÃ© J.V. Bertin

René J.V. Bertin

2017-08-02 10:15:56 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129324/
-----------------------------------------------------------

(Updated Aug. 2, 2017, 12:15 p.m.)

Status
------

This change has been discarded.

Review request for KDE Software on Mac OS X and KDevelop.

Repository: kdevplatform

Description
-------

Transferred to https://phabricator.kde.org/D5139

This patch proposes changes to the way the VCS commit message editor provides feedback concerning the message width.

KDE insists on a 72 character limit for commit messages, and that is probably a very reasonable standard. In the current implementation there is no easy way to judge how far from that threshold we are. The easiest way to do this is by setting the commit message editor to a width of 72 characters (which for some reason requires taking the width of a string of 74 characters). I think there is room enough for this, but a more complete implementation would probably introduce a resize handle between the 2 main widgets. I haven't yet tried to look into that.

Several approaches have been tried during the course of this RR.

Diffs
-----

plugins/git/gitmessagehighlighter.cpp da7660d3eebf9f836e49b48b41df2d11afe83268
vcs/widgets/vcsdiffpatchsources.cpp b080872d4366f65947244cd2c6299fbc14e9ea96

Diff: https://git.reviewboard.kde.org/r/129324/diff/

Testing
-------

On OS X with 2 different fixed font typefaces (sizes). The deleteLater() change has been in use since about 4.7.1 .

File Attachments
----------------

preview: 72-char wide commit message editor
https://git.reviewboard.kde.org/media/uploaded/files/2016/11/03/2ffdd73a-db19-4bf2-9545-7a850a4b7fd1__Screen_Shot_2016-11-03_at_18.24.54.png
commit message showing the overflow warnings/errors (taken on Linux)
https://git.reviewboard.kde.org/media/uploaded/files/2016/11/09/54eb4035-0fe1-4d6c-8365-db48bf67b826__KDev-Commit-Dialog.png

Thanks,

RenÃ© J.V. Bertin