<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/119234/">https://git.reviewboard.kde.org/r/119234/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 11th, 2014, 9:58 p.m. CEST, <b>Christian David</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 this only used until the pim libs are KF5 ready? Because I think this function is so important that it justifies having the pim libs a obligatory dependency. There will be many bug reports if a package maintainer forgets to compile with kdepimlibs.</p></pre>
</blockquote>
<p>On July 11th, 2014, 10:07 p.m. CEST, <b>Cristian Oneț</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;">No, I would like to see this as a permanent change. I'm not that sure about which of the features are so important that they must be a mandatory dependency. I like the idea of having the freedom to build without kdepimlibs if necessary and if that can be obtained easily why not have it? I'm not sugesting that packagers should do this but for example in source based distributions users would be able to have a choice, which is nice.</p></pre>
</blockquote>
<p>On July 11th, 2014, 10:12 p.m. CEST, <b>Alvaro Soliverez</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'm against this being a permanent change. It affects how schedules are handled, which impacts all over the application.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
And I certainly don't want to track down a bug reported by an unsupecting user just to figure out later a packager left the dependency out because kde pim libs was "too big".</p></pre>
</blockquote>
<p>On July 12th, 2014, 10:26 a.m. CEST, <b>Cristian Oneț</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 hate to disagree but I don't see it that way. The only effect to the way schedules are handled is that processing dates are not obtained from the holliday region thus the "old" (and documented) behavior of considering only weekends as non-processing days will be in effect.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If you ask me is this feature really that crucial that we should make everyone build kdepimlibs in oder to use kmymoney I would say no. Again, I'm not removing the feature, just making it optional.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The feature isn't even documented so I'm not sure if we would start receiving bug reports about it. But just to be on the safe side we could finish the feature in main.cpp:69 which would provide a list of optional features that were built (or found as plugins) in the about data.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If it's something I've learned from and appreacited in the KF5 model is that the take everything or leave it approach is wrong. Think about it, kdepimlibs has 22 libraries plus their dependecies , of which we only use 4 (QGpgme, KHolidays, PIMIdentities, Akonadi) for pretty slim features. Knowing this wouldn't it be wiser to allow those who don't need those features to be able to use the application without these dependencies?</p></pre>
</blockquote>
<p>On July 12th, 2014, 10:50 a.m. CEST, <b>Marko Käning</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;">Cristian, I am very much in favour for what you're stating here. This is something which one wouldn't be too worried about on Linux, but on OSX as on Windows in your case it is a real show-stopper to be forced into having far too many dependencies for seemingly little functionality gain.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Thus, I also vote for making things optional!</p></pre>
</blockquote>
<p>On July 12th, 2014, 4:28 p.m. CEST, <b>Christian David</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, on the long term kdepimlibs will be split up anyway. So the size problem might be solved just by waiting.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Please see my comments as not mainly targeting this review request but more as a general concern about making kdepimlibs optional.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">From my point of view this patch adds more complexity to KMyMoney without a real benefit.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">All future development needs to wrap the usage of kdepimlibs, which means a lot of work. Also it is hard for a new developer to understand/change it as he has to learn kdepimlibs and the KMyMoney wrapper. Also using #if HAVE_KDEPIMLIBS is uncomfortable from my point of view but maybe I am just too sensitive to that (I worked in project which overdid this — bad memories).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If this has a real benefit for Windows or Mac users it could be a fair deal until kdepimlibs got actually split up. Especially as long as it is a small patch like this one.</p></pre>
</blockquote>
<p>On July 12th, 2014, 4:41 p.m. CEST, <b>Cristian Oneț</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 agree that preprocessor definition based conditonal compiling is a PITA. KMyMoney already has KMM_DESIGNER which gave us a lot of headaches but in this case I think that there is a real benefit as long as the conditional code can be kept fairly simple and future development is not hindered.</p></pre>
</blockquote>
<p>On July 14th, 2014, 2:06 p.m. CEST, <b>Cristian Oneț</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;">This weekend I checked out the state of kde framemeworks 5 on Windows. Once I passed the hurdles and managed to install all the necessary frameworks (by fixing some issues) I would have found it much easier to start working on KMyMoney, if this patch would have been in master and frameworrks. Knowing the state of framworks kdepimlibs on Linux I didn't even tried to build it on Windows, just went ahead and ifdef-ed the kdepimlibs dependencies so I could get to the more important issue of KMyMoney on frameworks on Windows.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I guess the situation is similar on OS X. The sooner we would be able to setup a CI build of KMyMoney on all three systems (Linux, Windows, OS X) the better because KMyMoney could act as a real proof that frameworks are really cross platform like Qt.</p></pre>
</blockquote>
<p>On July 14th, 2014, 2:16 p.m. CEST, <b>Alvaro Soliverez</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's not a reason to have this as a permanent change. We found the same kind of issues during our port to Qt4, and these will be fixed too.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If this is going to go in, I'd like a very clear notice in the UI of the features disabled when the dependency is not included. No silent disabling.</p></pre>
</blockquote>
<p>On July 14th, 2014, 2:23 p.m. CEST, <b>Cristian Oneț</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;">OK then I'll prepare a patch that handles this using https://projects.kde.org/projects/extragear/office/kmymoney/repository/revisions/master/entry/kmymoney/main.cpp#L68 as a starting point.</p></pre>
</blockquote>
<p>On July 15th, 2014, 8:11 a.m. CEST, <b>Cristian Oneț</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 is https://git.reviewboard.kde.org/r/119287/</p></pre>
</blockquote>
<p>On July 15th, 2014, 1:31 p.m. CEST, <b>Alvaro Soliverez</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;">If this disables encryption too, shouldnt there be a change to prevent the UI from saving as (and opening) an encrypted file?</p></pre>
</blockquote>
<p>On July 15th, 2014, 2:10 p.m. CEST, <b>Cristian Oneț</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;">All GPG related functionality is already controlled by KGPGFile::GPGAvailable() so it's enough to return false from that method to disable GPG in the application.</p></pre>
</blockquote>
<p>On August 15th, 2014, 10:18 a.m. CEST, <b>Thomas Baumgart</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 we need to proceed with this one. I can imagine a clear dialog that pops up in case the user started a version w/o KDEPIM support showing the features that are not available. This message should come up every month or so, so that s/he is reminded about the fact. Somewhat <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">hiding</em> that information in the About dialog is not enough from my POV.</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;">Sounds good to me.</p></pre>
<br />
<p>- Marko</p>
<br />
<p>On July 11th, 2014, 9:58 p.m. CEST, Cristian Oneț 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 KMymoney.</div>
<div>By Cristian Oneț.</div>
<p style="color: grey;"><i>Updated July 11, 2014, 9:58 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kmymoney
</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;">If KDE PIM libraries will not be found during configuration all days will be considered processing days. Note that this review request dependes on the cmake changes in https://git.reviewboard.kde.org/r/119207/</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;">Build and run the application with and without HAVE_KDEPIMLIBS defined.</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>kmymoney/dialogs/settings/ksettingsschedules.cpp <span style="color: grey">(96bfb84)</span></li>
<li>kmymoney/kmymoney.cpp <span style="color: grey">(687187a)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/119234/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>