Re: Review Request 129192: Add namespace to desktop and appdata files

[Luigi Toscano]

> On Ott. 16, 2016, 12:06 p.m., Matěj Laitl wrote:
> > Looks good, but see one question below.
> > 
> > Also, I wonder what the consequences would be? People loosing custom-set 
> > file associations? What about shortcuts in panels etc.?
> 
> Matěj Laitl wrote:
> I also think MediaPlayer2::DesktopEntry() needs to be updated.

Right for MediaPlayer2.

It's possible that people would lose their favourite entry, if they linked to 
amarok desktop file. I'm not sure about associations, but I added the most 
knowledgable person about this to the review.


> On Ott. 16, 2016, 12:06 p.m., Matěj Laitl wrote:
> > src/CMakeLists.txt, lines 955-958
> > 
> >
> > What about these .desktop files? I seems that these should be moved, 
> > too, at least for consistency.

No, those are services. The specification talks about the reverse DNS syntax 
only for applications:
" For applications, the part of the name of the desktop file (before the 
.desktop) should follow the "reverse DNS" convention, e.g. 
org.example.FooViewer.desktop. "

https://specifications.freedesktop.org/desktop-entry-spec/latest/ar01s02.html


- Luigi


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


On Ott. 15, 2016, 9:44 p.m., Luigi Toscano wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129192/
> ---
> 
> (Updated Ott. 15, 2016, 9:44 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Repository: amarok
> 
> 
> Description
> ---
> 
> - add the organization namespace to the desktop and then appdata files, 
> according the specification;
> - use the new directory for metadata (metainfo) files.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 327ec10 
>   src/amarok.appdata.xml c580fd5 
>   src/amarok.desktop  
>   src/amarok_containers.desktop  
> 
> Diff: https://git.reviewboard.kde.org/r/129192/diff/
> 
> 
> Testing
> ---
> 
> Compiles, the files are installed in the expected places.
> 
> 
> Thanks,
> 
> Luigi Toscano
> 
>

Re: Review Request 129192: Add namespace to desktop and appdata files

[Luigi Toscano]

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

(Updated Ott. 16, 2016, 4:12 p.m.)


Review request for Amarok and Matthias Klumpp.


Changes
---

Really updated now, sorry for the spam.


Repository: amarok


Description
---

- add the organization namespace to the desktop and then appdata files, 
according the specification;
- use the new directory for metadata (metainfo) files.


Diffs (updated)
-

  src/CMakeLists.txt 327ec10 
  src/amarok.appdata.xml c580fd5 
  src/amarok.desktop  
  src/amarok_containers.desktop  
  src/dbus/mpris2/MediaPlayer2.cpp a1d1bda 

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


Testing
---

Compiles, the files are installed in the expected places.


Thanks,

Luigi Toscano

Re: Review Request 129192: Add namespace to desktop and appdata files

[Luigi Toscano]

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

(Updated Ott. 16, 2016, 4:10 p.m.)


Review request for Amarok and Matthias Klumpp.


Repository: amarok


Description
---

- add the organization namespace to the desktop and then appdata files, 
according the specification;
- use the new directory for metadata (metainfo) files.


Diffs (updated)
-

  src/CMakeLists.txt 327ec10 
  src/amarok.appdata.xml c580fd5 
  src/amarok.desktop  
  src/amarok_containers.desktop  

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


Testing
---

Compiles, the files are installed in the expected places.


Thanks,

Luigi Toscano

Re: Review Request 129192: Add namespace to desktop and appdata files

[Matěj Laitl]

> On Říj. 16, 2016, 12:06 odp., Matěj Laitl wrote:
> > Looks good, but see one question below.
> > 
> > Also, I wonder what the consequences would be? People loosing custom-set 
> > file associations? What about shortcuts in panels etc.?

I also think MediaPlayer2::DesktopEntry() needs to be updated.


- Matěj


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


On Říj. 15, 2016, 9:44 odp., Luigi Toscano wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129192/
> ---
> 
> (Updated Říj. 15, 2016, 9:44 odp.)
> 
> 
> Review request for Amarok.
> 
> 
> Repository: amarok
> 
> 
> Description
> ---
> 
> - add the organization namespace to the desktop and then appdata files, 
> according the specification;
> - use the new directory for metadata (metainfo) files.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 327ec10 
>   src/amarok.appdata.xml c580fd5 
>   src/amarok.desktop  
>   src/amarok_containers.desktop  
> 
> Diff: https://git.reviewboard.kde.org/r/129192/diff/
> 
> 
> Testing
> ---
> 
> Compiles, the files are installed in the expected places.
> 
> 
> Thanks,
> 
> Luigi Toscano
> 
>

Re: Review Request 129055: [amarok] Fixed Space key binded as a global shortcut (which breaks keyboard usage for the session).

[Matěj Laitl]

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


Ship it!




Looks good. Feel free to push if you have commit access. Since kf5 is a 
development branch and there are few people to review, feel free to push to it 
directly unless you explicitly want a review (perhaps for something 
controversial).

- Matěj Laitl


On Zář. 27, 2016, 10:13 odp., Alexandr Akulich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129055/
> ---
> 
> (Updated Zář. 27, 2016, 10:13 odp.)
> 
> 
> Review request for Amarok.
> 
> 
> Repository: amarok
> 
> 
> Description
> ---
> 
> There is a change at commit c3c4c7b8ebc4bc9ec94394f5d3a5569dee8b4725 :
> ```diff
>  setText( i18n( "Play/Pause" ) );
> -setShortcut( Qt::Key_Space );
> -setGlobalShortcut( KShortcut() );
> +KGlobalAccel::setGlobalShortcut(this, QKeySequence(Qt::Key_Space) );
> ```
> 
> The change globally binds Key_Space, which breaks the key usage for the 
> session.
> 
> 
> Diffs
> -
> 
>   src/ActionClasses.cpp 77334fc 
> 
> Diff: https://git.reviewboard.kde.org/r/129055/diff/
> 
> 
> Testing
> ---
> 
> Compiles.
> 
> 
> Thanks,
> 
> Alexandr Akulich
> 
>

Re: Review Request 129192: Add namespace to desktop and appdata files

[Matěj Laitl]

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



Looks good, but see one question below.

Also, I wonder what the consequences would be? People loosing custom-set file 
associations? What about shortcuts in panels etc.?


src/CMakeLists.txt (lines 955 - 958)


What about these .desktop files? I seems that these should be moved, too, 
at least for consistency.


- Matěj Laitl


On Říj. 15, 2016, 9:44 odp., Luigi Toscano wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129192/
> ---
> 
> (Updated Říj. 15, 2016, 9:44 odp.)
> 
> 
> Review request for Amarok.
> 
> 
> Repository: amarok
> 
> 
> Description
> ---
> 
> - add the organization namespace to the desktop and then appdata files, 
> according the specification;
> - use the new directory for metadata (metainfo) files.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 327ec10 
>   src/amarok.appdata.xml c580fd5 
>   src/amarok.desktop  
>   src/amarok_containers.desktop  
> 
> Diff: https://git.reviewboard.kde.org/r/129192/diff/
> 
> 
> Testing
> ---
> 
> Compiles, the files are installed in the expected places.
> 
> 
> Thanks,
> 
> Luigi Toscano
> 
>

Re: Review Request 129055: [amarok] Fixed Space key binded as a global shortcut (which breaks keyboard usage for the session).

[Alexandr Akulich]

> On Окт. 16, 2016, 3:12 п.п., Matěj Laitl wrote:
> > Looks good. Feel free to push if you have commit access. Since kf5 is a 
> > development branch and there are few people to review, feel free to push to 
> > it directly unless you explicitly want a review (perhaps for something 
> > controversial).

Thanks for the review, I have the access and will push the commit soon.


- Alexandr


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


On Сен. 28, 2016, 1:13 д.п., Alexandr Akulich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129055/
> ---
> 
> (Updated Сен. 28, 2016, 1:13 д.п.)
> 
> 
> Review request for Amarok.
> 
> 
> Repository: amarok
> 
> 
> Description
> ---
> 
> There is a change at commit c3c4c7b8ebc4bc9ec94394f5d3a5569dee8b4725 :
> ```diff
>  setText( i18n( "Play/Pause" ) );
> -setShortcut( Qt::Key_Space );
> -setGlobalShortcut( KShortcut() );
> +KGlobalAccel::setGlobalShortcut(this, QKeySequence(Qt::Key_Space) );
> ```
> 
> The change globally binds Key_Space, which breaks the key usage for the 
> session.
> 
> 
> Diffs
> -
> 
>   src/ActionClasses.cpp 77334fc 
> 
> Diff: https://git.reviewboard.kde.org/r/129055/diff/
> 
> 
> Testing
> ---
> 
> Compiles.
> 
> 
> Thanks,
> 
> Alexandr Akulich
> 
>

Re: Review Request 129192: Add namespace to desktop and appdata files

[Matthias Klumpp]

> On Oct. 16, 2016, 9:12 p.m., Matthias Klumpp wrote:
> > Please rename the .desktop files too, the name of the .desktop file and the 
> > value of the `` tag must match for this to work.
> > The DBus activation and Wayland window mapping will also only work with the 
> > renamed .desktop files.

Ah, I see it's renamed in the cmake install section - a bit unconventional, but 
it will work.


- Matthias


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


On Oct. 16, 2016, 2:12 p.m., Luigi Toscano wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129192/
> ---
> 
> (Updated Oct. 16, 2016, 2:12 p.m.)
> 
> 
> Review request for Amarok and Matthias Klumpp.
> 
> 
> Repository: amarok
> 
> 
> Description
> ---
> 
> - add the organization namespace to the desktop and then appdata files, 
> according the specification;
> - use the new directory for metadata (metainfo) files.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 327ec10 
>   src/amarok.appdata.xml c580fd5 
>   src/amarok.desktop  
>   src/amarok_containers.desktop  
>   src/dbus/mpris2/MediaPlayer2.cpp a1d1bda 
> 
> Diff: https://git.reviewboard.kde.org/r/129192/diff/
> 
> 
> Testing
> ---
> 
> Compiles, the files are installed in the expected places.
> 
> 
> Thanks,
> 
> Luigi Toscano
> 
>

Re: Review Request 129192: Add namespace to desktop and appdata files

[Matthias Klumpp]

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



Please rename the .desktop files too, the name of the .desktop file and the 
value of the `` tag must match for this to work.
The DBus activation and Wayland window mapping will also only work with the 
renamed .desktop files.

- Matthias Klumpp


On Oct. 16, 2016, 2:12 p.m., Luigi Toscano wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129192/
> ---
> 
> (Updated Oct. 16, 2016, 2:12 p.m.)
> 
> 
> Review request for Amarok and Matthias Klumpp.
> 
> 
> Repository: amarok
> 
> 
> Description
> ---
> 
> - add the organization namespace to the desktop and then appdata files, 
> according the specification;
> - use the new directory for metadata (metainfo) files.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 327ec10 
>   src/amarok.appdata.xml c580fd5 
>   src/amarok.desktop  
>   src/amarok_containers.desktop  
>   src/dbus/mpris2/MediaPlayer2.cpp a1d1bda 
> 
> Diff: https://git.reviewboard.kde.org/r/129192/diff/
> 
> 
> Testing
> ---
> 
> Compiles, the files are installed in the expected places.
> 
> 
> Thanks,
> 
> Luigi Toscano
> 
>

Re: Review Request 129192: Add namespace to desktop and appdata files

[Luigi Toscano]

> On Ott. 16, 2016, 11:12 p.m., Matthias Klumpp wrote:
> > Please rename the .desktop files too, the name of the .desktop file and the 
> > value of the `` tag must match for this to work.
> > The DBus activation and Wayland window mapping will also only work with the 
> > renamed .desktop files.
> 
> Matthias Klumpp wrote:
> Ah, I see it's renamed in the cmake install section - a bit 
> unconventional, but it will work.

The desktop files for Type=Application are renamed in the patch. The first 
question was about desktop files which represent Service. If I read the 
specification correctly, the requirement for the namespace is only for desktop 
files for applications, is it correct?

The second question is: how much does the change break stuff like customer-set 
file applications and/or shortcuts in panels?


- Luigi


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


On Ott. 16, 2016, 4:12 p.m., Luigi Toscano wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129192/
> ---
> 
> (Updated Ott. 16, 2016, 4:12 p.m.)
> 
> 
> Review request for Amarok and Matthias Klumpp.
> 
> 
> Repository: amarok
> 
> 
> Description
> ---
> 
> - add the organization namespace to the desktop and then appdata files, 
> according the specification;
> - use the new directory for metadata (metainfo) files.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 327ec10 
>   src/amarok.appdata.xml c580fd5 
>   src/amarok.desktop  
>   src/amarok_containers.desktop  
>   src/dbus/mpris2/MediaPlayer2.cpp a1d1bda 
> 
> Diff: https://git.reviewboard.kde.org/r/129192/diff/
> 
> 
> Testing
> ---
> 
> Compiles, the files are installed in the expected places.
> 
> 
> Thanks,
> 
> Luigi Toscano
> 
>