<table><tr><td style="">kossebau added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D16032">View Revision</a></tr></table><br /><div><div><blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D16032#341315" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D16032#341315</a>, <a href="https://phabricator.kde.org/p/apol/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@apol</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D16032#340778" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D16032#340778</a>, <a href="https://phabricator.kde.org/p/kossebau/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@kossebau</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D16032#340668" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D16032#340668</a>, <a href="https://phabricator.kde.org/p/apol/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@apol</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>Considering we're doing our own macro, we can be also more succint. I don't see why we can't have <tt style="background: #ebebeb; font-size: 13px;">declare_plugin_qt_logging_category(targetname)</tt></p>
<p>You can use <tt style="background: #ebebeb; font-size: 13px;">target_sources()</tt> instead of adding it to the variable.</p></div>
</blockquote>
<p>You mentioned something about target in an earlier comment. But I still have no idea what you mean here and what should be generated how, could you please give an explicit example, to help aligning my brain with yours? :)</p></div>
</blockquote>
<p>Does that make sense to you?</p></div>
</blockquote>
<p><snip sample></p>
<p>Ah , thanks, now I see, you meant the <em>plugin</em> target :)<br />
There is one blocker for this: some plugins share the generated debug.h and debug.cpp files between the actual plugin and some tests. In those cases adding the generated sources to the plugin target does not work (grep for "_LOG_" to find those places). I very much agree though that ideally all plugins should be made consistent here.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>Could possibly be called from <tt style="background: #ebebeb; font-size: 13px;">kdevplatform_add_plugin</tt>.</p></blockquote>
<p>Yes, that should make things more simple & consistent.</p>
<p>Please bear with this patch: its whole idea was to add the , not to fix the complete current logging category situation :)<br />
Let's fix things step-by-step.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D16032#inline-87532">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfunk</span> wrote in <span style="color: #4b4d51; font-weight: bold;">KDevelopMacrosInternal.cmake:143</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Any reason this is <tt style="background: #ebebeb; font-size: 13px;">macro()</tt>? Could be a <tt style="background: #ebebeb; font-size: 13px;">function()</tt>, no?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Macro, because we want to change a variable from the calling scope (the one whose name we get by <tt style="background: #ebebeb; font-size: 13px;">_sources</tt>, which is passed on to <tt style="background: #ebebeb; font-size: 13px;">ecm_qt_declare_logging_category</tt> which is the one actually going to change the very variable.)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D16032#inline-87529">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfunk</span> wrote in <span style="color: #4b4d51; font-weight: bold;">KDevelopMacrosInternal.cmake:143</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Way too much code duplication in all those <tt style="background: #ebebeb; font-size: 13px;">declare_*_qt_logging_category</tt> calls, IMO... But I realize that if you want to keep <tt style="background: #ebebeb; font-size: 13px;">_declare_qt_logging_category</tt> KDevelop-agnostic (for possible future upstreaming to ECM...) there's no way around this.</p>
<p style="padding: 0; margin: 8px;">If you don't want to keep <tt style="background: #ebebeb; font-size: 13px;">_declare_qt_logging_category</tt> KDevelop-agnostic you could add a <tt style="background: #ebebeb; font-size: 13px;">TYPE</tt> argument to it and set some defaults for the different <tt style="background: #ebebeb; font-size: 13px;">ARGS_*</tt> based on <tt style="background: #ebebeb; font-size: 13px;">TYPE</tt> instead. Thinking this out further, we wouldn't even need different <tt style="background: #ebebeb; font-size: 13px;">declare_*_qt_logging_category</tt>functions, but just use a <tt style="background: #ebebeb; font-size: 13px;">declare_qt_logging_category</tt> function with a <tt style="background: #ebebeb; font-size: 13px;">TYPE</tt> parameter directly. Just specifiy <tt style="background: #ebebeb; font-size: 13px;">TYPE ...</tt> at the call-site in the individual CMakeLists.txt files...</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">The TYPE seems a nice idea to save on the implementation side, will give a try.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R32 KDevelop</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D16032">https://phabricator.kde.org/D16032</a></div></div><br /><div><strong>To: </strong>kossebau, KDevelop<br /><strong>Cc: </strong>kfunk, apol, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd<br /></div>