<html>
 <body>
  <div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
   <table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
    <tr>
     <td>
      This is an automatically generated e-mail. To reply, visit:
      <a href="https://git.reviewboard.kde.org/r/129324/">https://git.reviewboard.kde.org/r/129324/</a>
     </td>
    </tr>
   </table>
   <br />










<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
 <p style="margin-top: 0;">On November 4th, 2016, 1:40 a.m. CET, <b>Aleix Pol Gonzalez</b> wrote:</p>
 <blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
  


<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
 <thead>
  <tr>
   <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
    <a href="https://git.reviewboard.kde.org/r/129324/diff/1/?file=483956#file483956line59" style="color: black; font-weight: bold; text-decoration: underline;">vcs/widgets/vcsdiffpatchsources.cpp</a>
    <span style="font-weight: normal;">

     (Diff revision 1)

    </span>
   </th>
  </tr>
 </thead>



 
 

 <tbody>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
    <td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
    <th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">59</font></th>
    <td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">    <span class="n">m_commitMessageEdit</span><span class="p">.</span><span class="n">data</span><span class="p">()</span><span class="o">-></span><span class="n">setMinimumSize</span><span class="p">(</span><span class="n">QSize</span><span class="p">(</span><span class="n">w72</span><span class="p">,</span><span class="mi">0</span><span class="p">));</span></pre></td>
  </tr>

 </tbody>

</table>

  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Shouldn't it be the preferred width?</p></pre>
 </blockquote>



 <p>On November 4th, 2016, 2:09 a.m. CET, <b>René J.V. Bertin</b> wrote:</p>
 <blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Exactly how does that change the likelihood that the widget really gets to be 72 characters wide?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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?)</p></pre>
 </blockquote>





 <p>On November 4th, 2016, 2:44 a.m. CET, <b>Aleix Pol Gonzalez</b> wrote:</p>
 <blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Here it's explained: http://doc.qt.io/qt-4.8/qwidget.html#size-hints-and-size-policies</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
 </blockquote>





 <p>On November 4th, 2016, 9:21 a.m. CET, <b>René J.V. Bertin</b> wrote:</p>
 <blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'll see if the parent widget width information is available (reliable) at this point so we can check the target widget against that.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">FWIW, I tried using <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">contentMargins()</code> 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 ;)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
 </blockquote>





 <p>On November 4th, 2016, 12:21 p.m. CET, <b>Aleix Pol Gonzalez</b> wrote:</p>
 <blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">We already have this: http://i.imgur.com/nyDOcrU.png</p></pre>
 </blockquote>





 <p>On November 4th, 2016, 2:37 p.m. CET, <b>René J.V. Bertin</b> wrote:</p>
 <blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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 <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">!warning</code>), but then the problem is to find a colour that keeps the text readable for all background colours, right?</p></pre>
 </blockquote>





 <p>On November 4th, 2016, 2:40 p.m. CET, <b>René J.V. Bertin</b> wrote:</p>
 <blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Too fast again:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">it seems that if I want to try the preferred size approach I'll have to subclass KTextEdit to give it a custom <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">sizeHint()</code> (or <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">minimumSizeHint()</code>) method, is that correct?</p></pre>
 </blockquote>





 <p>On November 4th, 2016, 3:16 p.m. CET, <b>Aleix Pol Gonzalez</b> wrote:</p>
 <blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
 </blockquote>





 <p>On November 4th, 2016, 3:50 p.m. CET, <b>René J.V. Bertin</b> wrote:</p>
 <blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
  <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Is that just a matter of replacing <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KTextEdit</code> with however a kate view is called internally, or something "a little" more complicated than that?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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 <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">additional</em> 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.</p></pre>
 </blockquote>







</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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 <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">vcsdiffpatchsources.cpp</code>). 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.</p></pre>
<br />




<p>- René J.V.</p>


<br />
<p>On November 3rd, 2016, 6:32 p.m. CET, René J.V. Bertin wrote:</p>








<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
 <tr>
  <td>

<div>Review request for KDE Software on Mac OS X and KDevelop.</div>
<div>By René J.V. Bertin.</div>


<p style="color: grey;"><i>Updated Nov. 3, 2016, 6:32 p.m.</i></p>









<div style="margin-top: 1.5em;">
 <b style="color: #575012; font-size: 10pt;">Repository: </b>
kdevplatform
</div>


<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
 <table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
 <tr>
  <td>
   <pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch proposes 2 point changes to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">VCSCommitDiffPatchSource</code>. 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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In order of appearance:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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 <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QObject::deleteLater()</code> for select widgets. That change was somehow never submitted for the commit message editor.</p></pre>
  </td>
 </tr>
</table>


<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
 <tr>
  <td>
   <pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">On OS X with 2 different fixed font typefaces (sizes). The deleteLater() change has been in use since about 4.7.1 .</p></pre>
  </td>
 </tr>
</table>


<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">

 <li>vcs/widgets/vcsdiffpatchsources.cpp <span style="color: grey">(42cf1d8)</span></li>

</ul>

<p><a href="https://git.reviewboard.kde.org/r/129324/diff/" style="margin-left: 3em;">View Diff</a></p>



<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>


 <li><a href="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">preview: 72-char wide commit message editor</a></li>

</ul>




  </td>
 </tr>
</table>







  </div>
 </body>
</html>