<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/125615/">https://git.reviewboard.kde.org/r/125615/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 13th, 2015, 4:26 a.m. CEST, <b>Martin Klapetek</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;">As a maintainer, I would be much much more happy if it actually had a check for win/mac rather than making audio notifications optional always. Also speaking as someone who deals with knotifications bug reports.</p></pre>
</blockquote>
<p>On October 13th, 2015, 4:33 a.m. CEST, <b>Martin Klapetek</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;">...and I'm sure there will be one about no sound in notifications in Plasma sooner or later, which I would like to avoid.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">(pressed enter too soon)</p></pre>
</blockquote>
<p>On October 13th, 2015, 7:10 a.m. CEST, <b>Sune Vuorela</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 like that things that aren't actually specific to an OS aren't marked as such, though one could also argue that it should be required unless passed -DDISABLE_PHONON or similar to cmake.</p></pre>
</blockquote>
<p>On October 13th, 2015, 7:28 a.m. CEST, <b>Christoph Cullmann</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;">Hi, I am pragmatic, I can guard it with "optional only on mac and win" (btw., no, you can't just remove the dep there, as you can build phonon if you really want on both platforms), but I don't think that is the right way. Neither the "-DISABLE..".</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If we want to make frameworks attractive, it must be easy to build them and easy to bundle them with your application.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">With the optional dependency it is.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If people build from source, you can anyway not avoid that they just compile a broken phonon or enable the -DISABLE.. switch.</p></pre>
</blockquote>
<p>On October 13th, 2015, 9 a.m. CEST, <b>Albert Astals Cid</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 think -DDISABLE_PHONON or similar (-DMAKE_PHONON_OPTIONAL) is the easiest way to make it so that people that don't want the dependency can still build but that it is a conscious decision so there's fewer people that complain later at a "no sounds in Plasma" level.</p></pre>
</blockquote>
<p>On October 13th, 2015, 9:03 a.m. CEST, <b>Christoph Cullmann</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;">But do we start to add such switches then for everything that we want to have optional? And how to document that? If we add just the hint to this switch to the description, people will just copy it and miscompile, too.
If we want to have our frameworks attractive for any non-linux person, I don't think that is the way to go.</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;"><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;">I like that things that aren't actually specific to an OS aren't marked as such</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The original proposal did mention "hassle on win/mac", so I understood the purpose of this patch to only allow easier building/bundling on win/mac.</p>
<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;">Hi, I am pragmatic</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Hi, I'm the maintainer who will be dealing with the bugs/issues this causes, which is virtually all event sounds in your desktop not working <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">and</em> not working without any apparent trouble (the sound is configured, the sound plays in the config etc). I appreciate the effort of wanting to make the frameworks more attractive to non-linux, but my job as a maintainer is to ensure things are bulletproof-good for current users, which are mostly linux/plasma ones. This is not entirely the best solution and it will break and I'll be the one dealing with it and I'd like to avoid it if possible. Which it is.</p>
<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;">But do we start to add such switches then for everything that we want to have optional?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't really want to have the sound optional though, at least not in Plasma; the audio part of KNotification is what plays the sound with various dialogs as well as the logout sound (and yes, people actually still do file reports for missing login/logout sound which wasn't there till 5.3 I think). So in this case, I think such switch would be better.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">But maybe I'm all wrong and no distro will actually break this (though looking at how ktp 15.08 went, I'm not hopeful at all). So tell you what, let's leave this in, but when a bug report about a missing sound arrives, I'll change it away to use a switch.</p></pre>
<br />
<p>- Martin</p>
<br />
<p>On October 12th, 2015, 11:01 p.m. CEST, Christoph Cullmann 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 Frameworks and Sune Vuorela.</div>
<div>By Christoph Cullmann.</div>
<p style="color: grey;"><i>Updated Oct. 12, 2015, 11:01 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
knotifications
</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;">Make phonon dependency optional, purely internal change, like it is done for speech.</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;">Still builds with (and now even without) phonon here.</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>CMakeLists.txt <span style="color: grey">(5a4ec83)</span></li>
<li>src/CMakeLists.txt <span style="color: grey">(2a88b5a)</span></li>
<li>src/knotificationmanager.cpp <span style="color: grey">(1c392b4)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/125615/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>