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

[WIP] RAIL notification icon support #5110

Open
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@hualet
Copy link
Contributor

hualet commented Dec 6, 2018

Now the implementation can work as:

  1. show a tray icon for remote apps.
  2. support basic button clicks.

but it also have known issues:

  1. the icon is poorly drawn, the background is black.
  2. the local tray icon windows sometimes are not embeded into the system tray.
  3. right click on the tray icon, the context menu shows at somewhere else.

trying to fix: #5108

@freerdp-bot

This comment has been minimized.

Copy link

freerdp-bot commented Dec 6, 2018

Can one of the admins verify this patch?

@@ -698,6 +708,23 @@ static BOOL convert_icon_color_to_argb(ICON_INFO* iconInfo, BYTE* argbPixels)
);
}

static void convert_argb_to_bgra(BYTE* argbPixels, BYTE* bgraPixels, int len)
{

This comment has been minimized.

@akallabeth

akallabeth Dec 6, 2018

Member

Why not use freerdp_image_copy?

int count;
ULONG_PTR* pKeys = NULL;
xfAppNotifyIcon* notifyIcon;
count = HashTable_GetKeys(xfc->railNotifyIcons, &pKeys);

This comment has been minimized.

@akallabeth

akallabeth Dec 6, 2018

Member

xfc->railNotifyIcons == NULL if not used with RAIL


if (orderInfo->fieldFlags & WINDOW_ORDER_STATE_NEW)
{
notifyIcon = (xfAppNotifyIcon*) calloc(1, sizeof(xfAppNotifyIcon));

This comment has been minimized.

@akallabeth

akallabeth Dec 6, 2018

Member

unchecked allocation

return;

XUnmapWindow(icon->xfc->display, icon->handle);
XDestroyWindow(icon->xfc->display, icon->handle);

This comment has been minimized.

@akallabeth

akallabeth Dec 6, 2018

Member

memory leak, xfAppNotifyIcon was allocated with calloc

if (!notifyIcon)
return FALSE;

HashTable_Add(xfc->railNotifyIcons, (void*)(UINT_PTR) orderInfo->notifyIconId,

This comment has been minimized.

@akallabeth

akallabeth Dec 6, 2018

Member

'0' is a valid notification ID but HashTable_Add will not accept that.

This comment has been minimized.

@hualet

hualet Dec 7, 2018

Contributor

Does that situation that '0' as a notification ID exist in practical ? How do I suppose to fix this ? 😅

This comment has been minimized.

@akallabeth

akallabeth Dec 7, 2018

Member

yes it does. @hardening pointed me in a simple direction, just set a comparison function that compares the pointers by type (UINT32) and use something like &notifyIcon->notifyIconId as key

This comment has been minimized.

@hualet

hualet Dec 9, 2018

Contributor

I made another pull request: #5120 , is that what you're pointing?

@akallabeth
Copy link
Member

akallabeth left a comment

Some glitches, but looks good overall.

  1. Biggest obstacle I've encountered is that you seem to do something wrong with the X11 calls, it reliably crashes Xwayland on fedora 29.
  2. The other thing that comes to mind is that you currently just positon a window at the bottom and are not using any notification / tray icon API provided, which might not work reliably.
@hardening

This comment has been minimized.

Copy link
Contributor

hardening commented Dec 6, 2018

@akallabeth could be nice to report the crash conditions to fedora (as an X client is not supposed to crash the whole server)

@akallabeth

This comment has been minimized.

Copy link
Member

akallabeth commented Dec 6, 2018

@hardening still trying to get a proper crash dump ;)

@akallabeth

This comment has been minimized.

Copy link
Member

akallabeth commented Dec 6, 2018

The tray icons should be set by libappindicator or similar and not as a simple X window if possible.

@hualet

This comment has been minimized.

Copy link
Contributor

hualet commented Dec 7, 2018

Thank you for your reviewing, I'll update the PR soon.

The tray icons should be set by libappindicator or similar and not as a simple X window if possible.

The reason that I didn't use libappindicator is that I don't know if it's proper to introduce new dependencies :P, if that's acceptable, I'll try that @akallabeth

@hualet

This comment has been minimized.

Copy link
Contributor

hualet commented Dec 7, 2018

Oh, libappindicator seems to be only working with GTK, so I don't think it's a very good choice.

@akallabeth

This comment has been minimized.

Copy link
Member

akallabeth commented Dec 7, 2018

@hualet problem with x11 windows is that they look like the app has 2 entries in most environments then but still no notification icon.
New deps are no problem, just check it still works without them activated ;)

@hualet

This comment has been minimized.

Copy link
Contributor

hualet commented Dec 7, 2018

New deps are no problem, just check it still works without them activated ;)

I'm reading gtk's source code to see how can I fix the tray icon problem.
because introducing libappindicator/gtk means we need another main loop, really overwhelming I think @akallabeth

xfc->scanline_pad, 0);

if (notifyIcon->image)
XDestroyImage(notifyIcon->image);

notifyIcon->image = image;
free(argbPixels);
// bgraPixels will be freed by XDestroyImage.

This comment has been minimized.

@akallabeth

akallabeth Dec 7, 2018

Member

No C++ comments please

@hualet hualet force-pushed the hualet:master branch from dcffffb to 926ade3 Dec 9, 2018

@hualet

This comment has been minimized.

Copy link
Contributor

hualet commented Dec 10, 2018

@akallabeth I've fixed all of the mistakes you mentioned above, except:

  1. hashtable don't accept uint32, which I've created a new PR #5120 trying to follow up the direction you told me.
  2. the tray icon background is black, I still don't have a very good way to fix that.

can you review my code again, thanks?

Fix notifyIconId used as key of HashTable
notifyIconId can be zero, but HashTable does't accept that.
@akallabeth

This comment has been minimized.

Copy link
Member

akallabeth commented Dec 10, 2018

@hualet

  • The hash table issue can be solved just like #5117
  • I'll have a look at it.
@hualet

This comment has been minimized.

Copy link
Contributor

hualet commented Dec 10, 2018

@hualet

  • The hash table issue can be solved just like #5117
  • I'll have a look at it.

Just went thru that PR and copied the solution :)

@akallabeth

This comment has been minimized.

Copy link
Member

akallabeth commented Dec 10, 2018

@hualet I've fixed a few remaining issues and rebased to current master. See https://github.com/akallabeth/FreeRDP/tree/rail_notify_icons (note: I squashed your changes to a single commit)

@hualet

This comment has been minimized.

Copy link
Contributor

hualet commented Dec 10, 2018

@hualet I've fixed a few remaining issues and rebased to current master. See https://github.com/akallabeth/FreeRDP/tree/rail_notify_icons (note: I squashed your changes to a single commit)

OK, so I need to close this PR now?

@akallabeth

This comment has been minimized.

Copy link
Member

akallabeth commented Dec 10, 2018

@hualet if you want to. I'm still struggling with getting the notification icon to work though.
I can see the window icon in the window bar (gnome that is with the extension for it) but:

  • No icon for the notification
  • Everything I expect from the notification icon is not working, aka context menu, ...
@hualet

This comment has been minimized.

Copy link
Contributor

hualet commented Dec 10, 2018

@hualet if you want to. I'm still struggling with getting the notification icon to work though.
I can see the window icon in the window bar (gnome that is with the extension for it) but:

  • No icon for the notification
  • Everything I expect from the notification icon is not working, aka context menu, ...

Hmmm.... I tested that with Mate, DDE and stalonetray, all seems good, can you test with stalonetray ?

@hualet

This comment has been minimized.

Copy link
Contributor

hualet commented Dec 10, 2018

Another thing I must mention is that the context menu is not working perfectly here too, the context menu shows at random position :(

@akallabeth

This comment has been minimized.

Copy link
Member

akallabeth commented Dec 10, 2018

@hualet I've tried with cinnamon and mate and only in mate I could see an icon (with a width of 1 pixel)

@hualet

This comment has been minimized.

Copy link
Contributor

hualet commented Dec 11, 2018

@hualet I've tried with cinnamon and mate and only in mate I could see an icon (with a width of 1 pixel)

Which application are you testing with? I can test it here too.

@hualet

This comment has been minimized.

Copy link
Contributor

hualet commented Dec 11, 2018

that's what it looks like in DDE:
image

@akallabeth

This comment has been minimized.

Copy link
Member

akallabeth commented Dec 11, 2018

@hualet Yes, with KDE plasma I can see an icon too. Application not relevant, all behave the same in this regard.
Still, I get many problems with nearly all WM so this will need some work before getting merged.

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