Re: Regarding RM #3316 Pgadmin4 No Tray Crash

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*