New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial support for 'com.canonical.Unity.LauncherEntry' APIs #590

Closed
wants to merge 12 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@azzar1
Contributor

azzar1 commented Sep 5, 2017

Initial support to 'com.canonical.Unity.LauncherEntry' APIs. At the moment it only supports the badge notification. The code can be tested using the following script: http://paste.ubuntu.com/25473314/

visual

Show outdated Hide outdated appIcons.js Outdated
Show outdated Hide outdated launcherAPI.js Outdated
@micheleg

This comment has been minimized.

Show comment
Hide comment
@micheleg

micheleg Sep 6, 2017

Owner

Hi Andrea, thanks for the PR. I tried to run your code and and found a couple for minor incompatibilities (I left a couple fo comments here). I wasn't able to test the badges with your script because I don't have libunity on my system at the moment... I will try to have a closer look in the next few days.

Owner

micheleg commented Sep 6, 2017

Hi Andrea, thanks for the PR. I tried to run your code and and found a couple for minor incompatibilities (I left a couple fo comments here). I wasn't able to test the badges with your script because I don't have libunity on my system at the moment... I will try to have a closer look in the next few days.

@azzar1

This comment has been minimized.

Show comment
Hide comment
@azzar1

azzar1 Sep 6, 2017

Contributor

Fixed thanks.

Contributor

azzar1 commented Sep 6, 2017

Fixed thanks.

@azzar1

This comment has been minimized.

Show comment
Hide comment
@azzar1

azzar1 Sep 20, 2017

Contributor

Any news?

Contributor

azzar1 commented Sep 20, 2017

Any news?

@azzar1

This comment has been minimized.

Show comment
Hide comment
@azzar1

azzar1 Sep 20, 2017

Contributor

I added support for progress bar too. The design is based on the one of Plank.

Contributor

azzar1 commented Sep 20, 2017

I added support for progress bar too. The design is based on the one of Plank.

@didrocks

This comment has been minimized.

Show comment
Hide comment
@didrocks

didrocks Sep 20, 2017

Collaborator

@micheleg & @franglais125: sorry to bother you with this, but do you have a moment for a review on Andrea's work? This is the last dock modification we would like to have for 17.10, and as you know, the timeline is short (sorry to come late with this) ;) Now that both indicator numbers and progress bar are included, it would be great to have your opinion on this.

If you don't have time, we can cherry-pick on our dock branch for now, still ensuring it goes upstream afterwards, as you prefer :)

Collaborator

didrocks commented Sep 20, 2017

@micheleg & @franglais125: sorry to bother you with this, but do you have a moment for a review on Andrea's work? This is the last dock modification we would like to have for 17.10, and as you know, the timeline is short (sorry to come late with this) ;) Now that both indicator numbers and progress bar are included, it would be great to have your opinion on this.

If you don't have time, we can cherry-pick on our dock branch for now, still ensuring it goes upstream afterwards, as you prefer :)

Show outdated Hide outdated appIcons.js Outdated
@@ -1636,6 +1668,7 @@ var DockManager = new Lang.Class({
Name: 'DashToDock.DockManager',
_init: function() {
this._remoteModel = new LauncherAPI.LauncherEntryRemoteModel();

This comment has been minimized.

@franglais125

franglais125 Sep 20, 2017

Contributor

I don't know that it is strictly necessary, but this object is never destroyed.

In Line ~1921 we can add this._remoteModel.destroy();, since the .destroy() method is already available.

@franglais125

franglais125 Sep 20, 2017

Contributor

I don't know that it is strictly necessary, but this object is never destroyed.

In Line ~1921 we can add this._remoteModel.destroy();, since the .destroy() method is already available.

@franglais125

This comment has been minimized.

Show comment
Hide comment
@franglais125

franglais125 Sep 20, 2017

Contributor

@azzar1 I got this running on 17.10. Nice work! Generally speaking, this is something that will only work in Ubuntu?

Unfortunately I run Debian, and I haven't found the libraries readily available.

@didrocks I will give it some testing (on 17.10) and see if I find problems/improvements. However, it's really up to @micheleg to plan the inclusion/path forward. Cheers!

Contributor

franglais125 commented Sep 20, 2017

@azzar1 I got this running on 17.10. Nice work! Generally speaking, this is something that will only work in Ubuntu?

Unfortunately I run Debian, and I haven't found the libraries readily available.

@didrocks I will give it some testing (on 17.10) and see if I find problems/improvements. However, it's really up to @micheleg to plan the inclusion/path forward. Cheers!

Show outdated Hide outdated launcherAPI.js Outdated
@azzar1

This comment has been minimized.

Show comment
Hide comment
@azzar1

azzar1 Sep 20, 2017

Contributor

Fixed.
Regarding the question "Generally speaking, this is something that will only work in Ubuntu?": Unfortunetly there is not a standard for this type of things, but most docks (like plank, etc.) supports the unity-api. It must be considered that using libunity is not compulsory as libunity is just a "wrapper" to easliy implement the required dbus interfaces.

Contributor

azzar1 commented Sep 20, 2017

Fixed.
Regarding the question "Generally speaking, this is something that will only work in Ubuntu?": Unfortunetly there is not a standard for this type of things, but most docks (like plank, etc.) supports the unity-api. It must be considered that using libunity is not compulsory as libunity is just a "wrapper" to easliy implement the required dbus interfaces.

@micheleg

This comment has been minimized.

Show comment
Hide comment
@micheleg

micheleg Sep 21, 2017

Owner
Owner

micheleg commented Sep 21, 2017

@azzar1

This comment has been minimized.

Show comment
Hide comment
@azzar1

azzar1 Sep 21, 2017

Contributor

It does not use ubuntu specific libraries. Everything is based on dbus.

Contributor

azzar1 commented Sep 21, 2017

It does not use ubuntu specific libraries. Everything is based on dbus.

@micheleg

This comment has been minimized.

Show comment
Hide comment
@micheleg

micheleg Sep 21, 2017

Owner
Owner

micheleg commented Sep 21, 2017

@franglais125

This comment has been minimized.

Show comment
Hide comment
@franglais125

franglais125 Sep 21, 2017

Contributor

@azzar1 Is there a way of easily running the dbus wrapper without installing libunity? I'm not familiar with the code, browsing through the source I found the unity-launcher.vala file, where I think the necessary api's are defined, but I'm not entirely sure.

I'm asking to know how difficult it would be to have this available on other systems.

Contributor

franglais125 commented Sep 21, 2017

@azzar1 Is there a way of easily running the dbus wrapper without installing libunity? I'm not familiar with the code, browsing through the source I found the unity-launcher.vala file, where I think the necessary api's are defined, but I'm not entirely sure.

I'm asking to know how difficult it would be to have this available on other systems.

@azzar1

This comment has been minimized.

Show comment
Hide comment
@azzar1

azzar1 Sep 22, 2017

Contributor

@franglais125 you need to implement inside the application a dbus server exporting the required interface with the wanted properties. libunity just does this. I don't have any code example but it's not complicated at all.

Contributor

azzar1 commented Sep 22, 2017

@franglais125 you need to implement inside the application a dbus server exporting the required interface with the wanted properties. libunity just does this. I don't have any code example but it's not complicated at all.

@micheleg

This comment has been minimized.

Show comment
Hide comment
@micheleg

micheleg Sep 24, 2017

Owner

I had a look at this. Overall I'm ok with it. I really need to rething the code organization ... but this is good enough for now. I've consolidated the history and made few cosmetic changes in the development branch. However, I could not check if this is working properly: I tried to use your script in a Ubuntu Artful virtual machine, but I could't see any notification badge, nor see any error. Any suggestion?

There's a couple of thing missing/tofix that probably do not directly affect the ubuntu side.

  1. Ideally I need a settings to switch on/off the feature. I can add this later.

  2. The way you set he default color of the badge. I'd keep a more agnostic one (maybe red) as a defualt. I think you should tweak it at the ubuntu theme level, rather than hardcode it here.This can be obviously an acceptable shortcut given your schedule.

If you confirm that the branch is indeed working, I'm happy to merge into master.

Owner

micheleg commented Sep 24, 2017

I had a look at this. Overall I'm ok with it. I really need to rething the code organization ... but this is good enough for now. I've consolidated the history and made few cosmetic changes in the development branch. However, I could not check if this is working properly: I tried to use your script in a Ubuntu Artful virtual machine, but I could't see any notification badge, nor see any error. Any suggestion?

There's a couple of thing missing/tofix that probably do not directly affect the ubuntu side.

  1. Ideally I need a settings to switch on/off the feature. I can add this later.

  2. The way you set he default color of the badge. I'd keep a more agnostic one (maybe red) as a defualt. I think you should tweak it at the ubuntu theme level, rather than hardcode it here.This can be obviously an acceptable shortcut given your schedule.

If you confirm that the branch is indeed working, I'm happy to merge into master.

@franglais125

This comment has been minimized.

Show comment
Hide comment
@franglais125

franglais125 Sep 24, 2017

Contributor

Here is a small thing to think about. I don't see it as a deal breaker, but still something that can be fixed:

screenshot from 2017-09-24 14-13-03

Perhaps we can toggle the notifications when showing the number overlays?

Edit: something like this: franglais125@2e6e28c

Contributor

franglais125 commented Sep 24, 2017

Here is a small thing to think about. I don't see it as a deal breaker, but still something that can be fixed:

screenshot from 2017-09-24 14-13-03

Perhaps we can toggle the notifications when showing the number overlays?

Edit: something like this: franglais125@2e6e28c

@azzar1

This comment has been minimized.

Show comment
Hide comment
@azzar1

azzar1 Sep 25, 2017

Contributor

@micheleg The branch is indeed working.

Contributor

azzar1 commented Sep 25, 2017

@micheleg The branch is indeed working.

@azzar1

This comment has been minimized.

Show comment
Hide comment
@azzar1

azzar1 Sep 25, 2017

Contributor

http://paste.ubuntu.com/25615125/ This a simple python script that can be used to test the code. This code does not uset libunity.

Contributor

azzar1 commented Sep 25, 2017

http://paste.ubuntu.com/25615125/ This a simple python script that can be used to test the code. This code does not uset libunity.

},
insertEntryRemote: function(remote) {
if (!remote || this._remoteEntries.includes(remote))

This comment has been minimized.

@franglais125

franglais125 Sep 25, 2017

Contributor

@azzar1 I used your new script (thanks!), and got this running on Gnome 3.22.

This line is throwing an error for me, since Debian is still on mozJS 24.

Perhaps we can change includes (which is not supported until mozJS 52) with:

if (!remote || this._remoteEntries.indexOf(remote) !== -1)
@franglais125

franglais125 Sep 25, 2017

Contributor

@azzar1 I used your new script (thanks!), and got this running on Gnome 3.22.

This line is throwing an error for me, since Debian is still on mozJS 24.

Perhaps we can change includes (which is not supported until mozJS 52) with:

if (!remote || this._remoteEntries.indexOf(remote) !== -1)
},
removeEntryRemote: function(remote) {
if (!remote || !this._remoteEntries.includes(remote))

This comment has been minimized.

@franglais125

franglais125 Sep 25, 2017

Contributor

Same here. includes -> indexOf

@franglais125

franglais125 Sep 25, 2017

Contributor

Same here. includes -> indexOf

This comment has been minimized.

@micheleg

micheleg Sep 25, 2017

Owner

Thanks.

@micheleg

micheleg Sep 25, 2017

Owner

Thanks.

@micheleg

This comment has been minimized.

Show comment
Hide comment
@micheleg

micheleg Sep 25, 2017

Owner

I also manage to get the badges to work with your new script. I've merged the branch into master. I've only skipped the commit changing the default colour, which you might want to keep in the ubuntu-branch for the time being.

Owner

micheleg commented Sep 25, 2017

I also manage to get the badges to work with your new script. I've merged the branch into master. I've only skipped the commit changing the default colour, which you might want to keep in the ubuntu-branch for the time being.

@micheleg

This comment has been minimized.

Show comment
Hide comment
@micheleg

micheleg Sep 25, 2017

Owner

I've seen you had already merged in the ubuntu branch and made a new release/announcement. Closing this pull request. Thanks for the contribution @azzar1 @didrocks @franglais125!

Reference to the commit in master: [5ef239d..a6fba76]

Owner

micheleg commented Sep 25, 2017

I've seen you had already merged in the ubuntu branch and made a new release/announcement. Closing this pull request. Thanks for the contribution @azzar1 @didrocks @franglais125!

Reference to the commit in master: [5ef239d..a6fba76]

@didrocks

This comment has been minimized.

Show comment
Hide comment
@didrocks

didrocks Sep 26, 2017

Collaborator

Thanks a lot as usual @micheleg, @franglais125 to have make that easy for ubuntu! I think @azzar1 will help you once we release 17.10 to get the final options and touches done! ;)

Collaborator

didrocks commented Sep 26, 2017

Thanks a lot as usual @micheleg, @franglais125 to have make that easy for ubuntu! I think @azzar1 will help you once we release 17.10 to get the final options and touches done! ;)

@gayanper

This comment has been minimized.

Show comment
Hide comment
@gayanper

gayanper Oct 12, 2017

Contributor

@micheleg Does the ubuntu feature (progress, badges) works on other distributions as well ? like antergos , fedora ?

Contributor

gayanper commented Oct 12, 2017

@micheleg Does the ubuntu feature (progress, badges) works on other distributions as well ? like antergos , fedora ?

@azzar1

This comment has been minimized.

Show comment
Hide comment
@azzar1

azzar1 Oct 12, 2017

Contributor
Contributor

azzar1 commented Oct 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment