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

[René J.V. Bertin]

> On Nov. 4, 2016, 1:40 a.m., Aleix Pol Gonzalez wrote:
> > 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?
> 
> René J.V. Bertin wrote:
>     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?)
> 
> Aleix Pol Gonzalez wrote:
>     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.


- René J.V.


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


On Nov. 3, 2016, 6:32 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. 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.
> 
> 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
> -----
> 
>   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/kde-mac/attachments/20161104/24bf778e/attachment-0001.html>