Re: Regarding RM #3316 Pgadmin4 No Tray Crash

Hi Dave,

I have changed the background colour on mouse hover for "pgAdmin4" menu.
Attached is the screenshot, please review it and let me know which colour
looks good or any other suggestion. I'll send the patch accordingly.

On Tue, Jul 17, 2018 at 4:42 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Thanks, patch applied.
>
> On Fri, Jul 13, 2018 at 7:10 AM, Akshay Joshi <
> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>
>> Attached is the refactored patch.
>>
>> On Fri, Jul 13, 2018 at 11:19 AM, Akshay Joshi <
>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>
>>>
>>>
>>> On Thu, Jul 12, 2018 at 9:38 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>
>>>>
>>>>
>>>> On Thu, Jul 12, 2018 at 4:40 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>
>>>>>
>>>>>
>>>>> On Thu, Jul 12, 2018 at 11:17 AM, Akshay Joshi <
>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Jul 11, 2018 at 8:18 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Wed, Jul 11, 2018 at 11:05 AM, Akshay Joshi <
>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>>
>>>>>>>> Hi Dave
>>>>>>>>
>>>>>>>> On Wed, Jul 11, 2018 at 1:31 PM, Dave Page <dpage(at)pgadmin(dot)org>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Wed, Jul 11, 2018 at 8:03 AM, Akshay Joshi <
>>>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>>>>
>>>>>>>>>> Hi
>>>>>>>>>>
>>>>>>>>>> On Tue, Jul 10, 2018 at 9:16 PM, Dave Page <dpage(at)pgadmin(dot)org>
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Tue, Jul 10, 2018 at 4:05 PM, Akshay Joshi <
>>>>>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On Tue, 10 Jul 2018, 18:32 Dave Page, <dpage(at)pgadmin(dot)org>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Hi
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Tue, Jul 10, 2018 at 11:20 AM, Akshay Joshi <
>>>>>>>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi Hackers,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I have implemented the fix for RM #3316 "Pgadmin4 No Tray
>>>>>>>>>>>>>> Crash". I have implemented it as follows:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> - Check the availability of System Tray for 30 seconds
>>>>>>>>>>>>>> (no changes made here).
>>>>>>>>>>>>>> - If System Tray not found create one Floating Window
>>>>>>>>>>>>>> with menu. (Refer attached screenshot).
>>>>>>>>>>>>>> - I have remove close(x) button of the floating window.
>>>>>>>>>>>>>> - Fedora have "Quit" menu for the applications, so I have
>>>>>>>>>>>>>> handle the close event and shutdown the python server.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I have tested this on Fedora-28 (no system tray) and Ubuntu
>>>>>>>>>>>>>> 18.04 (with system tray).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Please review the screenshot and suggest changes (if any).
>>>>>>>>>>>>>> I'll send the patch later.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> What does the window look like without the menu? I'd suggest
>>>>>>>>>>>>> putting a Slonik image there, and maybe a note (or a button to display a
>>>>>>>>>>>>> note) saying that installing a system tray plugin can be used to hide the
>>>>>>>>>>>>> window.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> You mean instead of "pgAdmin4" menu we should add toolbar
>>>>>>>>>>>> with button having slonik image and when click on there submenus will be
>>>>>>>>>>>> open.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> No - I assume that's a drop down menu over a blank window? I'm
>>>>>>>>>>> suggesting something to fill the blank space.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Attached are the screenshot after adding 'Slonik' icon and
>>>>>>>>>> 'Note'.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Looks good - though please fix the aspect ratio so it doesn't
>>>>>>>>> squish the logo. I may tweak the message in review.
>>>>>>>>>
>>>>>>>>
>>>>>>>> I have tried using "QPixmap", but not able to fix the aspect
>>>>>>>> ratio.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Should we allow user to resize the floating window? Definitely
>>>>>>>>>> not maximize, because icon gets blurred.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> No - but it should be minimisable.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Fixed. Attached is the working patch. Please review it.
>>>>>>>>
>>>>>>>
>>>>>>> Thanks. Here's an update to the patch which makes the floating
>>>>>>> window a Qt Form for ease of maintenance, as well as making a few other
>>>>>>> tweaks.
>>>>>>>
>>>>>>
>>>>>> I have applied the patch and found below issues:
>>>>>>
>>>>>> - Slonik icon is not visible, I have remove the QPixmap and apply
>>>>>> the style sheet in ui file, it works for me.
>>>>>>
>>>>>> I'll bet it was because I told it to use a local file, which it
>>>>> included without a path. I've seen Qt mess that up before.
>>>>>
>>>>>>
>>>>>> - Window is resizable, to fix that i have set the fixed size.
>>>>>>
>>>>>> OK.
>>>>>
>>>>>
>>>>>> Please refer "*pgAdmin4_Floating_Window.png*"
>>>>>>
>>>>>>>
>>>>>>> I really dislike the 30 second delay that's caused by the attempt to
>>>>>>> initialise the tray icon. We need to do something about that;
>>>>>>>
>>>>>>
>>>>>> Do we need to reduce the delay? Meanwhile I have added some action
>>>>>> messages like "Checking for system tray..." onto the splash screen, so the
>>>>>> user will be aware of the actions. Please refer "
>>>>>> *Splash_With_Message.png*"
>>>>>>
>>>>>
>>>>> Yes - we cannot have the check for a system tray taking 30 seconds.
>>>>> People get very annoyed when pgAdmin takes too long to start!
>>>>>
>>>>> As a worst case scenario, we can add a command line switch that tells
>>>>> it to not bother using the tray, which can then be used in the shortcut on
>>>>> the menu. However, I really would like to make this fully automatic, and
>>>>> near instantaneous if possible, so let's try to figure it out before
>>>>> falling back to that hack.
>>>>>
>>>>
>>>> I played some more. It looks like it's an easy fix - something like the
>>>> following seems to work for me on Fedora 28 (no tray) and macOS, with no
>>>> noticeable delay on either:
>>>>
>>>> if (!QSystemTrayIcon::isSystemTrayAvailable() || !trayicon->Init())
>>>>
>>>> {
>>>>
>>>> // Delete Tray Icon object
>>>>
>>>> delete trayicon;
>>>>
>>>> trayicon = NULL;
>>>>
>>>>
>>>> splash->showMessage(QString(QWidget::tr("System tray not found, creating floating window...")), Qt::AlignBottom | Qt::AlignCenter);
>>>>
>>>>
>>>> Of course, if we call QSystemTrayIcon::isSystemTrayAvailable()
>>>> earlier, we can skip even trying to create the tray icon, rather than
>>>> creating it and then deleting it. Can you look at refactoring that way, and
>>>> see if it works for you please?
>>>>
>>>
>>> Sure, If you can see the Init() method of trayicon it calls
>>> isSystemTrayAvailable() which eventually call QSystemTrayIcon::isSystemTrayAvailable()
>>> for 10 times in loop with wait of 3 seconds. Why we have added that logic?
>>> Is that method required to be called multiple times for the correct return
>>> value ? I am asking that because it would be easy for me to refactor the
>>> logic. If we can rely on single call to the function QSystemTrayIcon::
>>> isSystemTrayAvailable() then no need to create a tray icon if system
>>> tray is not available.
>>>
>>> Meanwhile I'll start refactoring the logic. Thanks
>>>
>>>
>>>
>>>>
>>>> Thanks.
>>>>
>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>> can we start the splash screen and server before trying to create
>>>>>>> the icon? That way the user can be up and running immediately; they just
>>>>>>> won't see the tray icon or floating window for a short while.
>>>>>>>
>>>>>>
>>>>>> Logic to show splash screen is already there and it is before
>>>>>> initializing the tray icon, but for some reason it is not visible. It is
>>>>>> visible only when app.exec() will be called. To fix that I'll have to add
>>>>>> sleep of 1 second before calling processEvents() (googled it).
>>>>>>
>>>>>
>>>>> We shouldn't need a sleep - simply processing events should be enough.
>>>>>
>>>>>
>>>>>>
>>>>>> Starting the server before trying to create tray icon shouldn't
>>>>>> be a good idea, because in case of some error in starting the server, user
>>>>>> should be able to view the log by clicking the "View log" menu from tray
>>>>>> icon.
>>>>>>
>>>>>
>>>>> Yeah - I'm not sure it's a huge issue as they'll be able to click
>>>>> something within 30 seconds, but it's certainly far from ideal.
>>>>>
>>>>> Thanks.
>>>>>
>>>>>
>>>>>>
>>>>>> Attached is the modified patch. Please review it.
>>>>>>
>>>>>>>
>>>>>>> Thanks.
>>>>>>>
>>>>>>> --
>>>>>>> Dave Page
>>>>>>> Blog: http://pgsnake.blogspot.com
>>>>>>> Twitter: @pgsnake
>>>>>>>
>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>>>> The Enterprise PostgreSQL Company
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> *Akshay Joshi*
>>>>>>
>>>>>> *Sr. Software Architect *
>>>>>>
>>>>>>
>>>>>>
>>>>>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246*
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Dave Page
>>>>> Blog: http://pgsnake.blogspot.com
>>>>> Twitter: @pgsnake
>>>>>
>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>> The Enterprise PostgreSQL Company
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Dave Page
>>>> Blog: http://pgsnake.blogspot.com
>>>> Twitter: @pgsnake
>>>>
>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>> The Enterprise PostgreSQL Company
>>>>
>>>
>>>
>>>
>>> --
>>> *Akshay Joshi*
>>>
>>> *Sr. Software Architect *
>>>
>>>
>>>
>>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246*
>>>
>>
>>
>>
>> --
>> *Akshay Joshi*
>>
>> *Sr. Software Architect *
>>
>>
>>
>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246*
>>
>
>
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

--
*Akshay Joshi*

*Sr. Software Architect *

*Phone: +91 20-3058-9517Mobile: +91 976-788-8246*