<table><tr><td style="">kossebau accepted this revision.<br />kossebau added a comment.<br />This revision is now accepted and ready to land.
</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/D24892">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/D24892#553597" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D24892#553597</a>, <a href="https://phabricator.kde.org/p/vkrause/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@vkrause</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/D24892#552838" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D24892#552838</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);"><p>Looks good. Perfect would be if you tested KIconThemes with EXCLUDE_DEPRECATED_BEFORE_AND_AT set to hexnumber(5.64.0), to see if there is no internal usage still happening, like with autotests which might need to support such a build with KICONTHEMES_ENABLE_DEPRECATED_SINCE (ENABLE variant, as external to lib).</p></div>
</blockquote>
<p>Assuming I used this correctly (setting EXCLUDE_DEPRECATED_BEFORE_AND_AT to 054000)</p></div>
</blockquote>
<p>Hm, guess the "default:0" confuses people, EXCLUDE_DEPRECATED_BEFORE_AND_AT would be set as string in majpr.minor.patch format ("0" & "CURRENT" are special cases). That is documented on <a href="https://api.kde.org/ecm/module/ECMGenerateExportHeader.html" class="remarkup-link" target="_blank" rel="noreferrer">https://api.kde.org/ecm/module/ECMGenerateExportHeader.html</a> but seemingly that is too far away, so will add to the option description text.</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>this doesn't build independent of assignIconsToContextMenu, due to the Q_ENUM code generated for KIconLoader still referencing the deprecated FileSystem enum element?</p></blockquote>
<p>Building with EXCLUDE_DEPRECATED_BEFORE_AND_AT set to 0, 4.8.0, 5.0.0 & CURRENT, to cover before & after versions, the build does not fail here.<br />
The generated Q_ENUM code should not see FileSystem because (and yes, the actual code written is confusing, no idea yet for better macros (names) which do not twist the mind), because:<br />
when setting EXCLUDE_DEPRECATED_BEFORE_AND_AT, this results in both KICONTHEMES_ENABLE_DEPRECATED_SINCE & KICONTHEMES_BUILD_DEPRECATED_SINCE to disable code based on the value of EXCLUDE_DEPRECATED_BEFORE_AND_AT. The difference between ENABLE & BUILD is, that developers linking against the built lib can control the ENABLE ones using the *_DISABLE_DEPRECATED_BEFORE_AND_AT, but not the BUILD one, that is fixed to the value set for EXCLUDE_DEPRECATED_BEFORE_AND_AT at generation time of the library.</p>
<p>Let's look at the code:</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">#if KICONTHEMES_ENABLE_DEPRECATED_SINCE(4, 8)
FileSystem, ///< An icon that represents a file system. @deprecated Since 4.8. Use Place instead.
#elif KICONTHEMES_BUILD_DEPRECATED_SINCE(4, 8)
FileSystem_DEPRECATED_DO_NOT_USE,
#endif</pre></div>
<p>When building KIconThemes, setting EXCLUDE_DEPRECATED_BEFORE_AND_AT to 4.8.0 or later, both conditions will not be met. So the compiler will skip any of enumerator variants. If setting to before 4.8.0, the first condition is already met during the build.</p>
<p>When building against KIconThemes, setting _KICONTHEMES_DISABLE_DEPRECATED_BEFORE_AND_AT now controls what the compiler sees. _KICONTHEMES_DISABLE_DEPRECATED_BEFORE_AND_AT though will be forced by the generated export header code to be at least at the version of EXCLUDE_DEPRECATED_BEFORE_AND_AT.<br />
With _KICONTHEMES_DISABLE_DEPRECATED_BEFORE_AND_AT being set to 4.8.0 and later, the first condition is not met, so the actual enumerator name is not available. To keep the following enumerators at the correct auto numbers (or if in need still have an enumerator which though has the warning in its name), there then is the elif branch, which will be only reached if enabled in the build.</p>
<p>Yes, very confusing by just looking at the raw code. Suggestions for better macro names (or better approaches) very welcome.</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/D24892#inline-140692">View Inline</a><span style="color: #4b4d51; font-weight: bold;">vkrause</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kicontheme.h:292</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I am unsure about that, therefore I'm opting for the safe approach :) It is IMHO possible some deprecate API will stay, e.g. if it got deprecated before having strict rules on porting away from such API or the porting impact turning out more complex than anticipated. I'm therefore only adding the KF6 todos in places where they could be executed immediately when disregarding BC.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Some API deprecated staying because porting away is not easy to do should be the exception though, no? <br />
I would rather flag only such API elements with a comment once it is found out in real world that porting is more hard than anticipated.<br />
But at the point of time of deprecation, the whole idea of the warnings being inserted into the compiler is to have people stop using it, so it can be removed at the next occasion, no? Otherwise the whole warning is harmful noise making people ignore warnings.</p>
<p style="padding: 0; margin: 8px;">And ideally the concept of deprecated-but-might-stay should be codified ideally in the macros, so the compiler can be taught to do what is wanted.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R302 KIconThemes</div></div></div><br /><div><strong>BRANCH</strong><div><div>master</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D24892">https://phabricator.kde.org/D24892</a></div></div><br /><div><strong>To: </strong>vkrause, kossebau<br /><strong>Cc: </strong>kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns<br /></div>