Re: Regarding RM #3316 Pgadmin4 No Tray Crash

On Thu, Jul 12, 2018 at 9:38 PM, Dave Page <dpage@pgadmin.org> wrote:

On Thu, Jul 12, 2018 at 4:40 PM, Dave Page <dpage@pgadmin.org> wrote:

On Thu, Jul 12, 2018 at 11:17 AM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

On Wed, Jul 11, 2018 at 8:18 PM, Dave Page <dpage@pgadmin.org> wrote:

On Wed, Jul 11, 2018 at 11:05 AM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Hi Dave

On Wed, Jul 11, 2018 at 1:31 PM, Dave Page <dpage@pgadmin.org> wrote:

On Wed, Jul 11, 2018 at 8:03 AM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Hi

On Tue, Jul 10, 2018 at 9:16 PM, Dave Page <dpage@pgadmin.org> wrote:

On Tue, Jul 10, 2018 at 4:05 PM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

On Tue, 10 Jul 2018, 18:32 Dave Page, <dpage@pgadmin.org> wrote:

Hi

On Tue, Jul 10, 2018 at 11:20 AM, Akshay Joshi <akshay.joshi@enterprisedb.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-9517
Mobile: +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-9517
Mobile: +91 976-788-8246