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

[René J.V. Bertin]

> On Nov. 8, 2016, 8:48 p.m., Aleix Pol Gonzalez wrote:
> > 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.


> On Nov. 8, 2016, 8:48 p.m., Aleix Pol Gonzalez wrote:
> > plugins/git/gitmessagehighlighter.cpp, line 41
> > <https://git.reviewboard.kde.org/r/129324/diff/2/?file=484065#file484065line41>
> >
> >     -1
> 
> René J.V. Bertin wrote:
>     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?
> 
> Aleix Pol Gonzalez wrote:
>     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
-----------------------------------------------------------


On Nov. 4, 2016, 6:07 p.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> 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.
> 
> 
> 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
> -----
> 
>   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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20161109/ed97b3d9/attachment-0001.html>