Re: Review Request 127655: Fix KAboutData::applicationData() to sync to current Q*Application metadata

[Friedrich W. H. Kossebau]

> On April 29, 2016, 3:15 a.m., Michael Pyne wrote:
> > I think I disagree with the idea of overwriting KAboutData properties if 
> > they are already set by the user. Alex, any thoughts?
> > 
> > In the event the KAboutData doesn't already exist I think automatically 
> > setting it up makes sense, and QCoreApplication is a good source. But I 
> > would rather flag property conflicts than to break ties when developer 
> > selects two different values for same property, as that's change in 
> > behavior might break other parts of code that rely on KAboutData not 
> > changing values.
> > 
> > Would this partial solution be OK for the problem you're running into?
> 
> Albert Astals Cid wrote:
> > I think I disagree with the idea of overwriting KAboutData properties 
> if they are already set by the user. Alex, any thoughts?
> 
> I agree with Michael, it seems strage it overwriting what you may have 
> set in setAboutData.
> 
> Friedrich W. H. Kossebau wrote:
> Hm. Would it not be more strange if one cannot be sure that 
> KAboutData::applicationData().displayName() matches 
> app->applicationDisplayName()? And similar for the other shared/mapped 
> application metadata properties? Why would I rather expect the original 
> property of the KAboutData value to be kept, than it being updated to its 
> counterpart in the Q*Application metadata, if changed by the Qt-API 
> afterwards?
> Surprised to see you expecting things to be different :)
> 
> Not that I expect it to happen a lot in real world code that the metadata 
> of applications is set/reset by independent code snippets (and thus a mixture 
> of KAboutData-using and Q*Application::setX-using), where random orders of 
> writing and reading the data can happen. But if it happens, should not both 
> Q*Application::setX and KAboutData::setApplicationData be equi-mighty, and 
> both be acting on the same effective properties when it comes to metadata 
> about the application instance, where it makes sense and is defined as such?
> 
> Or, why should KAboutData::setApplicationData be allowed to overrule any 
> already set QApplication metadata (which makes sense to everyone), but not 
> the other way around? :) Why should KAboutData::setApplicationData be 
> write-through, but KAboutData::applicationData not be read-through?
> 
> Like it is now, if I would want to use metadata of the application, I 
> cannot rely on KAboutData::applicationData alone, I also have to separately 
> read things by Q*Application::x, to be sure to really have the values also 
> used by other Qt-only parts of the code. Which renders the duplicates of 
> those Q*Application properties in KAboutData useless, no?
> I would expect more code to be broken if it relies only on 
> KAboutData::applicationData() than code which expects any properties to keep 
> their values, even if getting out-of-sync with the Qt application metadata.
> 
> For the bug which motivated me to write this patch, agreeing on 
> initialising at least the global KAboutData instance to current Q*Application 
> data should be fine, so will change the patch to that now. But well, I think 
> this initial patch should be the way to go and yield less surprising 
> behaviour :)
> 
> Albert Astals Cid wrote:
> > Would it not be more strange if one cannot be sure that 
> KAboutData::applicationData().displayName() matches 
> app->applicationDisplayName()?
> 
> To me, no.
> 
> > Why would I rather expect the original property of the KAboutData value 
> to be kept, than it being updated to its counterpart in the Q*Application 
> metadata, if changed by the Qt-API afterwards?
> 
> Why should they match? You're basically arguing for
> a->setX("BOO")
> b->setY("LALA")
> a->x() => should return "LALA"
> 
> That's the most strange API ever.
> 
> > Or, why should KAboutData::setApplicationData be allowed to overrule 
> any already set QApplication metadata (which makes sense to everyone), but 
> not the other way around?
> 
> I don't think it makes sense either. If we're to fix this assymetry, I 
> would much rather prefer a patch that doesn't overwrite QCoreApplication 
> version, name and organization domain if they have already been set.
> 
> I am not the maintainer of this framework though, so wait for a more 
> qualified opinion.

Well, I am arguing for what KAboutData::applicationData is about IMHO and 
expected by most code I have seen, which is:
a representation of the metadata of the Q*Application instance (which I 
consider a result of quick&dirty porting to Qt5 under the premise of API 
compatibility for all the existing codebase, not something which is well 
designed given the Qt5 API, in case you doubt). Do not agree with your code 
abstraction, but well, it's not straight object-oriented code in any case.

So what is the intend of any code which uses the setter/getter for that global 
KAboutD

Re: Review Request 127655: Fix KAboutData::applicationData() to sync to current Q*Application metadata

[Albert Astals Cid]

> On April 29, 2016, 3:15 a.m., Michael Pyne wrote:
> > I think I disagree with the idea of overwriting KAboutData properties if 
> > they are already set by the user. Alex, any thoughts?
> > 
> > In the event the KAboutData doesn't already exist I think automatically 
> > setting it up makes sense, and QCoreApplication is a good source. But I 
> > would rather flag property conflicts than to break ties when developer 
> > selects two different values for same property, as that's change in 
> > behavior might break other parts of code that rely on KAboutData not 
> > changing values.
> > 
> > Would this partial solution be OK for the problem you're running into?
> 
> Albert Astals Cid wrote:
> > I think I disagree with the idea of overwriting KAboutData properties 
> if they are already set by the user. Alex, any thoughts?
> 
> I agree with Michael, it seems strage it overwriting what you may have 
> set in setAboutData.
> 
> Friedrich W. H. Kossebau wrote:
> Hm. Would it not be more strange if one cannot be sure that 
> KAboutData::applicationData().displayName() matches 
> app->applicationDisplayName()? And similar for the other shared/mapped 
> application metadata properties? Why would I rather expect the original 
> property of the KAboutData value to be kept, than it being updated to its 
> counterpart in the Q*Application metadata, if changed by the Qt-API 
> afterwards?
> Surprised to see you expecting things to be different :)
> 
> Not that I expect it to happen a lot in real world code that the metadata 
> of applications is set/reset by independent code snippets (and thus a mixture 
> of KAboutData-using and Q*Application::setX-using), where random orders of 
> writing and reading the data can happen. But if it happens, should not both 
> Q*Application::setX and KAboutData::setApplicationData be equi-mighty, and 
> both be acting on the same effective properties when it comes to metadata 
> about the application instance, where it makes sense and is defined as such?
> 
> Or, why should KAboutData::setApplicationData be allowed to overrule any 
> already set QApplication metadata (which makes sense to everyone), but not 
> the other way around? :) Why should KAboutData::setApplicationData be 
> write-through, but KAboutData::applicationData not be read-through?
> 
> Like it is now, if I would want to use metadata of the application, I 
> cannot rely on KAboutData::applicationData alone, I also have to separately 
> read things by Q*Application::x, to be sure to really have the values also 
> used by other Qt-only parts of the code. Which renders the duplicates of 
> those Q*Application properties in KAboutData useless, no?
> I would expect more code to be broken if it relies only on 
> KAboutData::applicationData() than code which expects any properties to keep 
> their values, even if getting out-of-sync with the Qt application metadata.
> 
> For the bug which motivated me to write this patch, agreeing on 
> initialising at least the global KAboutData instance to current Q*Application 
> data should be fine, so will change the patch to that now. But well, I think 
> this initial patch should be the way to go and yield less surprising 
> behaviour :)

> Would it not be more strange if one cannot be sure that 
> KAboutData::applicationData().displayName() matches 
> app->applicationDisplayName()?

To me, no.

> Why would I rather expect the original property of the KAboutData value to be 
> kept, than it being updated to its counterpart in the Q*Application metadata, 
> if changed by the Qt-API afterwards?

Why should they match? You're basically arguing for
a->setX("BOO")
b->setY("LALA")
a->x() => should return "LALA"

That's the most strange API ever.

> Or, why should KAboutData::setApplicationData be allowed to overrule any 
> already set QApplication metadata (which makes sense to everyone), but not 
> the other way around?

I don't think it makes sense either. If we're to fix this assymetry, I would 
much rather prefer a patch that doesn't overwrite QCoreApplication version, 
name and organization domain if they have already been set.

I am not the maintainer of this framework though, so wait for a more qualified 
opinion.


- Albert


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127655/#review95002
---


On April 28, 2016, 1:04 a.m., Friedrich W. H. Kossebau wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127655/
> ---
> 
> (Updated April 28, 2016, 1:04 a.m.)
> 
> 
> Review request for KDE Frameworks, Alex Richardson and Michael Pyne.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
>

Re: Review Request 127655: Fix KAboutData::applicationData() to sync to current Q*Application metadata

[Friedrich W. H. Kossebau]

> On April 29, 2016, 3:15 a.m., Michael Pyne wrote:
> > I think I disagree with the idea of overwriting KAboutData properties if 
> > they are already set by the user. Alex, any thoughts?
> > 
> > In the event the KAboutData doesn't already exist I think automatically 
> > setting it up makes sense, and QCoreApplication is a good source. But I 
> > would rather flag property conflicts than to break ties when developer 
> > selects two different values for same property, as that's change in 
> > behavior might break other parts of code that rely on KAboutData not 
> > changing values.
> > 
> > Would this partial solution be OK for the problem you're running into?
> 
> Albert Astals Cid wrote:
> > I think I disagree with the idea of overwriting KAboutData properties 
> if they are already set by the user. Alex, any thoughts?
> 
> I agree with Michael, it seems strage it overwriting what you may have 
> set in setAboutData.

Hm. Would it not be more strange if one cannot be sure that 
KAboutData::applicationData().displayName() matches 
app->applicationDisplayName()? And similar for the other shared/mapped 
application metadata properties? Why would I rather expect the original 
property of the KAboutData value to be kept, than it being updated to its 
counterpart in the Q*Application metadata, if changed by the Qt-API afterwards?
Surprised to see you expecting things to be different :)

Not that I expect it to happen a lot in real world code that the metadata of 
applications is set/reset by independent code snippets (and thus a mixture of 
KAboutData-using and Q*Application::setX-using), where random orders of writing 
and reading the data can happen. But if it happens, should not both 
Q*Application::setX and KAboutData::setApplicationData be equi-mighty, and both 
be acting on the same effective properties when it comes to metadata about the 
application instance, where it makes sense and is defined as such?

Or, why should KAboutData::setApplicationData be allowed to overrule any 
already set QApplication metadata (which makes sense to everyone), but not the 
other way around? :) Why should KAboutData::setApplicationData be 
write-through, but KAboutData::applicationData not be read-through?

Like it is now, if I would want to use metadata of the application, I cannot 
rely on KAboutData::applicationData alone, I also have to separately read 
things by Q*Application::x, to be sure to really have the values also used by 
other Qt-only parts of the code. Which renders the duplicates of those 
Q*Application properties in KAboutData useless, no?
I would expect more code to be broken if it relies only on 
KAboutData::applicationData() than code which expects any properties to keep 
their values, even if getting out-of-sync with the Qt application metadata.

For the bug which motivated me to write this patch, agreeing on initialising at 
least the global KAboutData instance to current Q*Application data should be 
fine, so will change the patch to that now. But well, I think this initial 
patch should be the way to go and yield less surprising behaviour :)


- Friedrich W. H.


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127655/#review95002
---


On April 28, 2016, 1:04 a.m., Friedrich W. H. Kossebau wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127655/
> ---
> 
> (Updated April 28, 2016, 1:04 a.m.)
> 
> 
> Review request for KDE Frameworks, Alex Richardson and Michael Pyne.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> KAboutData is passed as values on getting and setting the "applicationData",
> and it only makes sense to have its properties be a transparent access
> to the actual mirrored Q*Application metadata.
> 
> Even more as there is code in KF5 (e.g. KXMLGUI) which relies on 
> KAboutData::applicationData(),
> without requiring the user to use KAboutData::setApplicationData().
> 
> 
> Diffs
> -
> 
>   autotests/kaboutdatatest.cpp f31e7f3 
>   src/lib/kaboutdata.h 97c0f2b 
>   src/lib/kaboutdata.cpp ceb0c06 
> 
> Diff: https://git.reviewboard.kde.org/r/127655/diff/
> 
> 
> Testing
> ---
> 
> Added autotests pass.
> 
> 
> Thanks,
> 
> Friedrich W. H. Kossebau
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127655: Fix KAboutData::applicationData() to sync to current Q*Application metadata

[Albert Astals Cid]

> On April 29, 2016, 3:15 a.m., Michael Pyne wrote:
> > I think I disagree with the idea of overwriting KAboutData properties if 
> > they are already set by the user. Alex, any thoughts?
> > 
> > In the event the KAboutData doesn't already exist I think automatically 
> > setting it up makes sense, and QCoreApplication is a good source. But I 
> > would rather flag property conflicts than to break ties when developer 
> > selects two different values for same property, as that's change in 
> > behavior might break other parts of code that rely on KAboutData not 
> > changing values.
> > 
> > Would this partial solution be OK for the problem you're running into?

> I think I disagree with the idea of overwriting KAboutData properties if they 
> are already set by the user. Alex, any thoughts?

I agree with Michael, it seems strage it overwriting what you may have set in 
setAboutData.


- Albert


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127655/#review95002
---


On April 28, 2016, 1:04 a.m., Friedrich W. H. Kossebau wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127655/
> ---
> 
> (Updated April 28, 2016, 1:04 a.m.)
> 
> 
> Review request for KDE Frameworks, Alex Richardson and Michael Pyne.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> KAboutData is passed as values on getting and setting the "applicationData",
> and it only makes sense to have its properties be a transparent access
> to the actual mirrored Q*Application metadata.
> 
> Even more as there is code in KF5 (e.g. KXMLGUI) which relies on 
> KAboutData::applicationData(),
> without requiring the user to use KAboutData::setApplicationData().
> 
> 
> Diffs
> -
> 
>   autotests/kaboutdatatest.cpp f31e7f3 
>   src/lib/kaboutdata.h 97c0f2b 
>   src/lib/kaboutdata.cpp ceb0c06 
> 
> Diff: https://git.reviewboard.kde.org/r/127655/diff/
> 
> 
> Testing
> ---
> 
> Added autotests pass.
> 
> 
> Thanks,
> 
> Friedrich W. H. Kossebau
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127655: Fix KAboutData::applicationData() to sync to current Q*Application metadata

[Michael Pyne]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127655/#review95002
---



I think I disagree with the idea of overwriting KAboutData properties if they 
are already set by the user. Alex, any thoughts?

In the event the KAboutData doesn't already exist I think automatically setting 
it up makes sense, and QCoreApplication is a good source. But I would rather 
flag property conflicts than to break ties when developer selects two different 
values for same property, as that's change in behavior might break other parts 
of code that rely on KAboutData not changing values.

Would this partial solution be OK for the problem you're running into?


src/lib/kaboutdata.h (line 319)


I'd rather this say something along lines of:

"If setApplicationData has not been called, the returned KAboutData will be 
initialized from equivalent information in the QCoreApplication instance, if 
present. See setApplicationData for the list of properties."



src/lib/kaboutdata.h (line 332)


And desktopFileName



src/lib/kaboutdata.cpp (line 920)


I think an else block should be added here that verifies whether the 
QCoreApplication properties are consistent with KAboutData's and if not, 
displays a big huge warning for each conflicting property.

I'd rather not initialize KAboutData from QCoreApplication if doing so 
would overwrite a non-empty KAboutData property, at least at this point. Seems 
to risk a very spooky 'action-at-a-distance' in  code whose behavior would then 
depend on when you ran KAboutData::setApplicationData in relation to creating a 
QCoreApplication.



src/lib/kaboutdata.cpp (line 923)


On other hand, initializing a KAboutData that hasn't already been set from 
QCoreApplication makes sense. But I would move this into the if block above so 
that it only runs when a KAboutData wasn't already available.


- Michael Pyne


On April 28, 2016, 1:04 a.m., Friedrich W. H. Kossebau wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127655/
> ---
> 
> (Updated April 28, 2016, 1:04 a.m.)
> 
> 
> Review request for KDE Frameworks, Alex Richardson and Michael Pyne.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> KAboutData is passed as values on getting and setting the "applicationData",
> and it only makes sense to have its properties be a transparent access
> to the actual mirrored Q*Application metadata.
> 
> Even more as there is code in KF5 (e.g. KXMLGUI) which relies on 
> KAboutData::applicationData(),
> without requiring the user to use KAboutData::setApplicationData().
> 
> 
> Diffs
> -
> 
>   autotests/kaboutdatatest.cpp f31e7f3 
>   src/lib/kaboutdata.h 97c0f2b 
>   src/lib/kaboutdata.cpp ceb0c06 
> 
> Diff: https://git.reviewboard.kde.org/r/127655/diff/
> 
> 
> Testing
> ---
> 
> Added autotests pass.
> 
> 
> Thanks,
> 
> Friedrich W. H. Kossebau
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 127655: Fix KAboutData::applicationData() to sync to current Q*Application metadata

[Friedrich W. H. Kossebau]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127655/
---

Review request for KDE Frameworks.


Repository: kcoreaddons


Description
---

KAboutData is passed as values on getting and setting the "applicationData",
and it only makes sense to have its properties be a transparent access
to the actual mirrored Q*Application metadata.

Even more as there is code in KF5 (e.g. KXMLGUI) which relies on 
KAboutData::applicationData(),
without requiring the user to use KAboutData::setApplicationData().


Diffs
-

  autotests/kaboutdatatest.cpp f31e7f3 
  src/lib/kaboutdata.h 97c0f2b 
  src/lib/kaboutdata.cpp ceb0c06 

Diff: https://git.reviewboard.kde.org/r/127655/diff/


Testing
---

Added autotests pass.


Thanks,

Friedrich W. H. Kossebau

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel