Re: Review Request 126660: Avoid finding the same package multiple times from different paths.

[Andreas Hartmetz]

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

(Updated Jan. 8, 2016, 7:11 p.m.)


Review request for KDE Frameworks, kdelibs, Plasma, and Marco Martin.


Changes
---

Don't look the category anymore and reduce the amount of indirection, now more 
feasible with the previous simplification.


Repository: kpackage


Description
---

That was a problem in a scenario such as mine, where I have distro
packages and self compiled packages. There were (from the UI)
indistinguishable, duplicate packages in the panel's add applets
UI, in the tray config dialog's "additional items" section, and
probably in other places, too.
Note that requiring to have no duplicate packages in XDG_DATA_DIRS
by removing entries from XGD_DATA_DIRS doesn't fly because /usr
cannot be removed for the non-KF5 things that it brings.


Diffs (updated)
-

  src/kpackage/packageloader.cpp 8b802d6 

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


Testing
---

Checked for duplicate entries in "add applets" and tray config dialog, no 
duplicates anymore. Also no duplicates anymore in KWin effects KCM.


Thanks,

Andreas Hartmetz

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

Re: Review Request 126660: Avoid finding the same package multiple times from different paths.

[Marco Martin]

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

Ship it!


Ship It!

- Marco Martin


On Jan. 8, 2016, 7:11 p.m., Andreas Hartmetz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126660/
> ---
> 
> (Updated Jan. 8, 2016, 7:11 p.m.)
> 
> 
> Review request for KDE Frameworks, kdelibs, Plasma, and Marco Martin.
> 
> 
> Repository: kpackage
> 
> 
> Description
> ---
> 
> That was a problem in a scenario such as mine, where I have distro
> packages and self compiled packages. There were (from the UI)
> indistinguishable, duplicate packages in the panel's add applets
> UI, in the tray config dialog's "additional items" section, and
> probably in other places, too.
> Note that requiring to have no duplicate packages in XDG_DATA_DIRS
> by removing entries from XGD_DATA_DIRS doesn't fly because /usr
> cannot be removed for the non-KF5 things that it brings.
> 
> 
> Diffs
> -
> 
>   src/kpackage/packageloader.cpp 8b802d6 
> 
> Diff: https://git.reviewboard.kde.org/r/126660/diff/
> 
> 
> Testing
> ---
> 
> Checked for duplicate entries in "add applets" and tray config dialog, no 
> duplicates anymore. Also no duplicates anymore in KWin effects KCM.
> 
> 
> Thanks,
> 
> Andreas Hartmetz
> 
>

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

Re: Review Request 126660: Avoid finding the same package multiple times from different paths.

[Andreas Hartmetz]

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

(Updated Jan. 8, 2016, 11:29 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, kdelibs, Plasma, and Marco Martin.


Changes
---

Submitted with commit 85cc90b65ed8a9aa110018b01813fe1fd6772d48 by Andreas 
Hartmetz to branch master.


Repository: kpackage


Description
---

That was a problem in a scenario such as mine, where I have distro
packages and self compiled packages. There were (from the UI)
indistinguishable, duplicate packages in the panel's add applets
UI, in the tray config dialog's "additional items" section, and
probably in other places, too.
Note that requiring to have no duplicate packages in XDG_DATA_DIRS
by removing entries from XGD_DATA_DIRS doesn't fly because /usr
cannot be removed for the non-KF5 things that it brings.


Diffs
-

  src/kpackage/packageloader.cpp 8b802d6 

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


Testing
---

Checked for duplicate entries in "add applets" and tray config dialog, no 
duplicates anymore. Also no duplicates anymore in KWin effects KCM.


Thanks,

Andreas Hartmetz

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

Re: Review Request 126660: Avoid finding the same package multiple times from different paths.

[Andreas Hartmetz]

> On Jan. 7, 2016, 3:31 p.m., Sebastian Kügler wrote:
> > src/kpackage/packageloader.cpp, line 190
> > 
> >
> > What does the category have to do with this? We should only be going by 
> > the id (the plugin name).
> 
> Andreas Hartmetz wrote:
> Depends on the scope in which an ID is guaranteed to be unique. If it's 
> guaranteed to be globally unique and independent of category, sure, I'm all 
> for not making it more complicated than necessary.

Given that the ID falls back to the metadata file name without extension, and 
that you can have the same file name in different directories, it seems like 
you *could* have a package of the same name in different categories. The same 
goes for names supplied by the data in the metadata files. Some names make 
sense in several places.


- Andreas


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


On Jan. 7, 2016, 3:21 p.m., Andreas Hartmetz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126660/
> ---
> 
> (Updated Jan. 7, 2016, 3:21 p.m.)
> 
> 
> Review request for KDE Frameworks, kdelibs, Plasma, and Marco Martin.
> 
> 
> Repository: kpackage
> 
> 
> Description
> ---
> 
> That was a problem in a scenario such as mine, where I have distro
> packages and self compiled packages. There were (from the UI)
> indistinguishable, duplicate packages in the panel's add applets
> UI, in the tray config dialog's "additional items" section, and
> probably in other places, too.
> Note that requiring to have no duplicate packages in XDG_DATA_DIRS
> by removing entries from XGD_DATA_DIRS doesn't fly because /usr
> cannot be removed for the non-KF5 things that it brings.
> 
> 
> Diffs
> -
> 
>   src/kpackage/packageloader.cpp 9f7dd48 
> 
> Diff: https://git.reviewboard.kde.org/r/126660/diff/
> 
> 
> Testing
> ---
> 
> Checked for duplicate entries in "add applets" and tray config dialog, no 
> duplicates anymore. Also no duplicates anymore in KWin effects KCM.
> 
> 
> Thanks,
> 
> Andreas Hartmetz
> 
>

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

Re: Review Request 126660: Avoid finding the same package multiple times from different paths.

[Sebastian Kügler]

> On Jan. 7, 2016, 3:31 p.m., Sebastian Kügler wrote:
> > src/kpackage/packageloader.cpp, line 190
> > 
> >
> > What does the category have to do with this? We should only be going by 
> > the id (the plugin name).
> 
> Andreas Hartmetz wrote:
> Depends on the scope in which an ID is guaranteed to be unique. If it's 
> guaranteed to be globally unique and independent of category, sure, I'm all 
> for not making it more complicated than necessary.
> 
> Andreas Hartmetz wrote:
> Given that the ID falls back to the metadata file name without extension, 
> and that you can have the same file name in different directories, it seems 
> like you *could* have a package of the same name in different categories. The 
> same goes for names supplied by the data in the metadata files. Some names 
> make sense in several places.
> 
> Andreas Hartmetz wrote:
> Sorry, the part about the filename doesn't apply. It comes from the 
> documentation of KPluginMetaData::pluginId() which doesn't apply here, given 
> that the files are all called metadata.desktop or metadata.json.

Yes, and the directory name is actually the same as the plugin name -- you can 
rely on that. So just a stringlist would in fact already be enough, but as I 
said ... why not use lst to check if it's already in there. Performance, I 
guess?

But yes, the only thing we need to look at is the plugin id. Nothing else 
matters. Well, we need to make sure that the most-local plugin is actually put 
in the list, otherwise you can't override with locally installed ones.


> On Jan. 7, 2016, 3:31 p.m., Sebastian Kügler wrote:
> > src/kpackage/packageloader.cpp, line 187
> > 
> >
> > This doesn't actually add anything. A better name would IMO be: 
> > alreadyListed() or something along those lines.
> 
> Andreas Hartmetz wrote:
> It also adds it to the list, though.
> 
> Andreas Hartmetz wrote:
> The point is that it isn't const, and it also has a return value that 
> matters. The name -sort of- conveys both.

That list is internal and shouldn't be reflected in the API, though... (We may 
entirely circumven this issue, see below.)


- Sebastian


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


On Jan. 7, 2016, 3:21 p.m., Andreas Hartmetz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126660/
> ---
> 
> (Updated Jan. 7, 2016, 3:21 p.m.)
> 
> 
> Review request for KDE Frameworks, kdelibs, Plasma, and Marco Martin.
> 
> 
> Repository: kpackage
> 
> 
> Description
> ---
> 
> That was a problem in a scenario such as mine, where I have distro
> packages and self compiled packages. There were (from the UI)
> indistinguishable, duplicate packages in the panel's add applets
> UI, in the tray config dialog's "additional items" section, and
> probably in other places, too.
> Note that requiring to have no duplicate packages in XDG_DATA_DIRS
> by removing entries from XGD_DATA_DIRS doesn't fly because /usr
> cannot be removed for the non-KF5 things that it brings.
> 
> 
> Diffs
> -
> 
>   src/kpackage/packageloader.cpp 9f7dd48 
> 
> Diff: https://git.reviewboard.kde.org/r/126660/diff/
> 
> 
> Testing
> ---
> 
> Checked for duplicate entries in "add applets" and tray config dialog, no 
> duplicates anymore. Also no duplicates anymore in KWin effects KCM.
> 
> 
> Thanks,
> 
> Andreas Hartmetz
> 
>

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

Re: Review Request 126660: Avoid finding the same package multiple times from different paths.

[Andreas Hartmetz]

> On Jan. 7, 2016, 3:31 p.m., Sebastian Kügler wrote:
> > src/kpackage/packageloader.cpp, line 190
> > 
> >
> > What does the category have to do with this? We should only be going by 
> > the id (the plugin name).
> 
> Andreas Hartmetz wrote:
> Depends on the scope in which an ID is guaranteed to be unique. If it's 
> guaranteed to be globally unique and independent of category, sure, I'm all 
> for not making it more complicated than necessary.
> 
> Andreas Hartmetz wrote:
> Given that the ID falls back to the metadata file name without extension, 
> and that you can have the same file name in different directories, it seems 
> like you *could* have a package of the same name in different categories. The 
> same goes for names supplied by the data in the metadata files. Some names 
> make sense in several places.

Sorry, the part about the filename doesn't apply. It comes from the 
documentation of KPluginMetaData::pluginId() which doesn't apply here, given 
that the files are all called metadata.desktop or metadata.json.


- Andreas


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


On Jan. 7, 2016, 3:21 p.m., Andreas Hartmetz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126660/
> ---
> 
> (Updated Jan. 7, 2016, 3:21 p.m.)
> 
> 
> Review request for KDE Frameworks, kdelibs, Plasma, and Marco Martin.
> 
> 
> Repository: kpackage
> 
> 
> Description
> ---
> 
> That was a problem in a scenario such as mine, where I have distro
> packages and self compiled packages. There were (from the UI)
> indistinguishable, duplicate packages in the panel's add applets
> UI, in the tray config dialog's "additional items" section, and
> probably in other places, too.
> Note that requiring to have no duplicate packages in XDG_DATA_DIRS
> by removing entries from XGD_DATA_DIRS doesn't fly because /usr
> cannot be removed for the non-KF5 things that it brings.
> 
> 
> Diffs
> -
> 
>   src/kpackage/packageloader.cpp 9f7dd48 
> 
> Diff: https://git.reviewboard.kde.org/r/126660/diff/
> 
> 
> Testing
> ---
> 
> Checked for duplicate entries in "add applets" and tray config dialog, no 
> duplicates anymore. Also no duplicates anymore in KWin effects KCM.
> 
> 
> Thanks,
> 
> Andreas Hartmetz
> 
>

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

Re: Review Request 126660: Avoid finding the same package multiple times from different paths.

[Andreas Hartmetz]

> On Jan. 7, 2016, 3:31 p.m., Sebastian Kügler wrote:
> > Wouldn't it be way easier to simply check if the plugin is already in lst?
> > 
> > Your approach looks very complex for just that...

O(n) vs. O(n^2)


- Andreas


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


On Jan. 7, 2016, 3:21 p.m., Andreas Hartmetz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126660/
> ---
> 
> (Updated Jan. 7, 2016, 3:21 p.m.)
> 
> 
> Review request for KDE Frameworks, kdelibs, Plasma, and Marco Martin.
> 
> 
> Repository: kpackage
> 
> 
> Description
> ---
> 
> That was a problem in a scenario such as mine, where I have distro
> packages and self compiled packages. There were (from the UI)
> indistinguishable, duplicate packages in the panel's add applets
> UI, in the tray config dialog's "additional items" section, and
> probably in other places, too.
> Note that requiring to have no duplicate packages in XDG_DATA_DIRS
> by removing entries from XGD_DATA_DIRS doesn't fly because /usr
> cannot be removed for the non-KF5 things that it brings.
> 
> 
> Diffs
> -
> 
>   src/kpackage/packageloader.cpp 9f7dd48 
> 
> Diff: https://git.reviewboard.kde.org/r/126660/diff/
> 
> 
> Testing
> ---
> 
> Checked for duplicate entries in "add applets" and tray config dialog, no 
> duplicates anymore. Also no duplicates anymore in KWin effects KCM.
> 
> 
> Thanks,
> 
> Andreas Hartmetz
> 
>

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