<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/122660/">https://git.reviewboard.kde.org/r/122660/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 21st, 2015, 2:44 p.m. CET, <b>Hugo Pereira Da Costa</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;">"Preinstalled colours schemes to ensure consistent colouring"
that is a regression with respect to exiting code. Its a no go. Please include.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"Updated the behaviour of the resize grip - fixed bug for fullscreen"
Can you post a bug report related to the bug ? There was indeed an issue with size grip and fullscreen window, but this was fixed, as far as I know. See https://bugs.kde.org/show_bug.cgi?id=343988 and commit therein</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"Redesigned buttons and tweaked the titlebar slightly."
Can you post screenshots ? Were these changes discussed in the Visual Design Group forum ? (https://forum.kde.org/viewforum.php?f=285)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"Font weight will now affect the boldness of icons in buttons"
Same question.
No other element in the widget style has this behavior (icons, frame sizes, etc.), so I'd like to know the rationale behind it and get this discussed.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I have not tested the changes yet, but will do asap</p></pre>
</blockquote>
<p>On February 21st, 2015, 4:24 p.m. CET, <b>Ken Vermette</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;">"that is a regression with respect to exiting code. Its a no go. Please include."
Sorry, I should have said "tested with preinstalled colour schemes"; I was just checking to ensure colours were sane for themes; make sure the buttons didn't turn neon-pink when using obsidian or anything like that.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"There was indeed an issue with size grip and fullscreen window ..."
I re-wrote the grip behaviour with the input of Jens (another VDG member); I wasn't specifically aiming to do it with the change to the grip display code, but I was aware that it would be fixed in my build. The new behaviour should hide the grip on maximised windows and inactive windows; on maximised windows the grip is pointless, and on inactive windows the grip is not obvious, but still covers scrollbars or status items...</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"Can you post screenshots ?"
Of course; they're included now.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"Were these changes discussed in the Visual Design Group forum ?"
Not on the forum, no. I did refer to Jens during the sprint, and other members of the VDG have responded positivly to my screenshot on G+. This is also an initial version, and assuming it flies I'll be updating based on feedback accordingly. The design is also evolutionary anyway.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"No other element in the widget style has this behavior (icons, frame sizes, etc.), so I'd like to know the rationale behind it and get this discussed."
I was going to add a config option but if I had of done this, but I chose to use the font width to avoid overburdening the settings dialog with tweaker stuff - it's less than a 0.7px difference.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'd be perfectly fine removing the feature; when first starting I did not know there was a float variant of setWidth - so I could not get the line to a happy medium; now that I have it at a 'good' width it was kept simply because it could 'follow along' with the font easily. The edit is <1 line.</p></pre>
</blockquote>
<p>On February 21st, 2015, 5:41 p.m. CET, <b>Hugo Pereira Da Costa</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;">"Preinstalled colours schemes to ensure consistent colouring"
Sorry. That was a wrong copy paste (damn review board).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I meant: "Note; Buttons are not animated yet in this variant"
that is a regression with respect to exiting code. Its a no go. Please include.
I am ok with incremental commits but not when they remove existing and working functionalities. Re-added animations can be the topic of another review request, but this one will not get a ship it untill the other one exists, sorry.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">""There was indeed an issue with size grip and fullscreen window ..."
I re-wrote the grip behaviour with the input of Jens (another VDG member); I wasn't specifically aiming to do it with the change to the grip display code, but I was aware that it would be fixed in my build. The new behaviour should hide the grip on maximised windows and inactive windows; on maximised windows the grip is pointless, and on inactive windows the grip is not obvious, but still covers scrollbars or status items..."
So, this is not a bug fix (since the bug is already fixed), this is a behavorial change. Please take it out of this commit and submit it in a separate review request for further discussion. It is independent of the design change. Note that some people (me for instance) could see the hiding of the resize handle on inactive window as a regression, (because of not being able to resize inactive windows any more). </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">""Were these changes discussed in the Visual Design Group forum ?" Not on the forum, no. I did refer to Jens during the sprint, and other members of the VDG have responded positivly to my screenshot on G+. This is also an initial version, and assuming it flies I'll be updating based on feedback accordingly. The design is also evolutionary anyway."</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I would really appreciate a dedicated thread on the design changes on the forum, if only to get feedback of other forum members, including me.
I can post some of my personnal comments here (and copy there when the thread exist)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">-- I find the colors too distracting. (but that is only personnal, possibly biased, so feel free to ignore unless many more people feel the same, especially other 'design' experts</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I fail to see the rationale behind the button coloring. I understand there is some sort of 'traffic light' metaphor, but then, why would minimize be more "orange" than say "unpin", or "keep below", or "un-maximize". Likewise, why would maximize be more "green" than say "pin", or "keep above". </p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">with the current code close button appears as "grey" on active window and red on others. While the other buttons are colored on active and grey on inactive. I fail to see why. To me at least, "close" is as dangerous on active windows than on inactive, while making it grey would make it "less" important than other coloured buttons. </p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">with the current code the "pressed button" click state feedback coloring feel somewhat random: grey on active window, "highlight" on inactive. </p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I fail to see the added value of the circles around the buttons, especially when there is already a nice mouse-over effect. It would be like adding a frame around every single toolbutton in a toolbar, around the icon. As a side effect, for a given button size, it also makes the actual glyphs 2 pixels smaller than what they are now, due to the need to the extra spacing with respect to the circle. Which, IMHO goes in the wrong direction. </p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">on the redesign of the "pin" button, I fail to see what issue the new design address with respect to the old one. </p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">finally, the same buttons are also used in the widget style, for dock manipulations (see: dolphin -> unlock panels, as well as kdevelop), and MDI windows (see oxygen-demo5, mdi window tab). So any change to the decoration should also come with a matching change to the widget style. </p>
</li>
</ul>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That's all I have for now.
Again:
- I'd rather have these things discussed on the forum rather than here
- I have nothing against incremental commits, but not when the first one break things that working and against which there have been no bugs reported nor formal complains.
- I would encourage you to commit these changes to a dedicated branch or scratch repository, ask for feedback, fix the issues that show up (especially the one that are not present in the current code), and when ready (e.g. no regression, inconsistencies, and overall consensus here and with VDG members), merge via review-request.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Best,</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Hugo</p></pre>
</blockquote>
</blockquote>
<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;">Should this be discussed then? I didn't see a discussion.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think it's a shame that these initiatives are just discarded instead of iterated. We shouldn't consider everything as Done.</p></pre>
<br />
<p>- Aleix</p>
<br />
<p>On March 16th, 2015, 3:44 a.m. CET, Ken Vermette 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 Plasma and Hugo Pereira Da Costa.</div>
<div>By Ken Vermette.</div>
<p style="color: grey;"><i>Updated March 16, 2015, 3:44 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
breeze
</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;">Overhaul of the Breeze Window decoration drawing code;
- Redesigned buttons and tweaked the titlebar slightly.
- Updated the behaviour of the resize grip - fixed bug for fullscreen
- Font weight will now affect the boldness of icons in buttons</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Note; Buttons are not animated yet in this variant.</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;"><ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Tried out preinstalled colours schemes to ensure consistent colouring</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Viewed various button sizes</li>
</ul></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>kdecoration/breezebutton.cpp <span style="color: grey">(5ac0cfe)</span></li>
<li>kdecoration/breezedecoration.h <span style="color: grey">(9eb6c65)</span></li>
<li>kdecoration/breezedecoration.cpp <span style="color: grey">(b474a8b)</span></li>
<li>kdecoration/breezebutton.h <span style="color: grey">(c43959a)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/122660/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/2015/02/21/66f8d3d8-5852-4b79-b211-339cbc7bf712__newdecos.png">Updated Decos</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/02/21/20580b13-3bb1-4020-9ddf-8d1253184a45__snapshot1.png">Full Windows</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>