Re: Regarding RM #3316 Pgadmin4 No Tray Crash

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*