Re: Regarding RM #3316 Pgadmin4 No Tray Crash

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