Animate activestate on frameless dialog boxes.

[ntoll]

Description

This is a work in progress (WiP).

NEEDS UX FEEDBACK / CHECKING

Fixes #927 Fixes #928 Fixes #912.

OK... here's a screenie of the work so far.

Things to note:

• The spinner @ninavizz has supplied doesn't do infinite looping. I've used the GIMP to re-export the GIF with infinite looping and it looks like this. I just want to check you're OK with the result.

• What you're seeing is my "pretend" solution where I put the "CONTINUE" button into the animated state upon dialog opening. My solution is generic enough that all such dialogs (print and usb export) will be able to start and stop this animation at the appropriate points in the dialog's lifecycle. These screenies are just for illustrative purposes.

• These dialogs behave very weird for me -- I'm unable to ALT-TAB away to another window (such as the one I'm using to record my screenies). This feels to me like a UX bad smell.

Please can @ninavizz / @eloquence feedback on the look of the work done so far and, once OK I'll make the necessary changes so rather than animating on dialog start, it comes into effect at the appropriate point in the lifecycle of the dialog.

Test Plan

I've added appropriate unit tests for the new methods in the base FramelessDialog class. Please can @ninavizz @eloquence provide UX/UI feedback on this WiP.

I've not tested on Qubes.

Checklist

If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:

• I have tested these changes in the appropriate Qubes environment
• I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
• These changes should not need testing in Qubes

If these changes add or remove files other than client code, packaging logic (e.g., the AppArmor profile) may need to be updated. Please check as applicable:

• I have submitted a separate PR to the packaging repo
• No update to the packaging logic (e.g., AppArmor profile) is required for these changes
• I don't know and would appreciate guidance

[ninavizz]

Hi Nicholas! Quick/initial feedback points:

• There's no outline on the Active State button. The fill should also be #f1f1f6 per the Zeplin.
• The spinner size is also shown in Zeplin. If there's a dev reason that cannot be attained, we should troubleshoot that. It's currently too small to be clearly visible... and it also visually "stutters."
• This spinner looks much too thin, too. Is it the correct one that was spec'd for just the button, and not for the top-left corner of this screen?

My own software on this end is set to generate infinitely looping animations, so @creviera since we're on the same timezone... maybe you or @eloquence could look into this with me? I know they aren't spinning in the GitHub previews, but I did not expect they would also fail to spin in the code. :/

[sssoleileraaa]

These dialogs behave very weird for me -- I'm unable to ALT-TAB away to another window (such as the one I'm using to record my screenies). This feels to me like a UX bad smell.

The dialogs should not set the WindowFlag.Popup as we discussed in the last UX meeting, but that change will need to be tested in Qubes and remember that we will see the window frame. If this is getting in your way of working on active state for refresh, please let me know and we can prioritize the dialog change from qt popup to regular old window dialog with its own close button.

[sssoleileraaa]

Just a note: If we use setIcon to add an icon to the button, it will appear smaller than the design, but I think this is a good first step. Eventually we'll need to make our own QPushButton class that allows for larger icons, also see: #679 (comment)

[ntoll]

Preach it sister..! 🙂

Small steps, etc...

[ninavizz]

Additional thing I also just noticed @ntoll: the screen shown in this PR never gets a spinner on the button. Only in the top left corner. Just as the mockups in Zeplin show. This is the one screen that shows the primary button in a regular disabled state, whereas the rest of the screens in the print and export flows, should show a spinner active-state button. :)

[ntoll]

This is the one screen that shows the primary button in a regular disabled state, whereas the rest of the screens in the print and export flows, should show a spinner active-state button. :)

As I said, what you're seeing is my "pretend" implementation screen so you can see the spinner in-situ but on the wrong screen (Qubes testing is hard for me, as also mentioned).

I'll fix the UI nitpicks mentioned above and ensure the spinner only happens at the right moments in the workflow.

ATTENTION someone (not me?) will need to test this in Qubes.

[ntoll]

• There's no outline on the Active State button. The fill should also be #f1f1f6
  per the Zeplin.

The background IS #f1f1f6 per the Zeplin and I'll remove the outline. Nice spot.

• The spinner size is also shown in Zeplin. If there's a dev reason that cannot be attained, we
  should troubleshoot that. It's currently too small to be clearly visible... and it also visually "stutters."

Stuttering is due to my GIF capture. It's smooth on my desktop.

I should be able to update the spinner size on the button in my code.

• This spinner looks much too thin, too. Is it the correct one that was spec'd for just the button,
  and not for the top-left corner of this screen?

OK... there have been so many spinner images I'll need to check. In any case, I'll endeavour to use the "wider" version.

My own software on this end is set to generate infinitely looping animations, so @creviera since
we're on the same timezone... maybe you or @eloquence could look into this with me? I know they
aren't spinning in the GitHub previews, but I did not expect they would also fail to spin in the code.
:/

None of the spinners infinitely loop for me. To fix this I import them into GIMP and re-export them with the "infinitely loop" flag set to true. Then they work.

[ntoll]

Visual updates to the active-state button:

• Confirm correct background.
• Correct QSize(21, 21) for the QIcon associated with the button (using the dimensions from the Zeplin).
• Remove border from active button.

Need to address the spinner in the top left corner.

[ninavizz]

Hey Nicholas! Sorry, I didn't realize Qubes was required to do this on a different screen... :D

Could the spinner be increased in size by ~120%? It still appears too small, even though I don't doubt you checked it correctly against the Zeplin.

[ntoll]

@ninavizz you tell me the pixel dimensions you want, and I'd be happy to update. 😄

[ntoll]

May I suggest a nice round number like 32x32..?

[ninavizz]

As it's a "proportional" issue... how tall are the buttons in the code?

[ninavizz]

Punting on refining the size of the spinner in this PR, per #945. This looks great and gets my signoff, @ntoll! :)

[ntoll]

OK folks, this is getting close to being over the line. Here's what I've done:

• Correct spinners.
• Methods on the base class which starts and stops two sorts of animation: activestate (for the primary activity button) and header (for the large icon in the header).
• Various minor CSS changes to make ^^^ look pertty.

I've also added the appropriate calls to the export file pane so that the activestate starts and stops at the appropriate times in the workflow.

PROBLEM I can't make sense of this Zeplin to know when to start/stop the header spinner. The dialogs shown don't match with what's currently on master. For instance, there's no "Ready to Export" stage (as far as I can see -- grepping for the text doesn't throw up anything).
Some guidance here would be most welcome. To be explicit it'd be wonderful if I could see dialogs that actually look like the current state of master (so I can see where I'm supposed to change) and a full account of the state changes for the print related dialog.

Basically, the heavy lifting is done, but I'm not sure where to activate / deactivate the various animations at the various points in the state changes (and this isn't clear from the Zeplins which don't accurately follow what's currently in master). Once I have this information I'd love to finish this all off.

Finally, once done, this'll need testing in Qubes.

[ninavizz]

Hey friend! Apologies my Zeplin was unclear. :/

Stuff was split between the Issue #927 and the Zeplin... but the most significant bit was in the Issue itself, which is that probably a separate PR will need to be created to create that second state. You are correct, that second state does not currently exist. @creviera or @eloquence advisement on how to split this between Nicholas and Qubes-capable peeps?

[ntoll]

OK... so since I'm not developing on Qubes I'll need some guidance for when to:

• activate the spinner in the export dialog (I could guess by name, but would like to know for sure). See this Zeplin for what I mean. @creviera your name was mentioned as someone who could point this out.
• I should also update the label one the export is ready (so it matches the final state in the Zeplin inked to above).

Then we'll be ready for review. Hurrah..!

[sssoleileraaa]

Just poking my head in here and looks like active state icon on the "Continue" button doesn't go away once the button is enabled. You can test this on non-qubes by applying this diff (check it out on master and your branch to see how before the button would enabled and show the "Continue" text):

--- a/securedrop_client/logic.py
+++ b/securedrop_client/logic.py
@@ -668,7 +668,9 @@ class Controller(QObject):
         logger.info('Running printer preflight check')
 
         if not self.qubes:
-            self.export.printer_preflight_success.emit()
+            import threading
+            threading.Timer(3, self.export.printer_preflight_success.emit).start()
+            # self.export.printer_preflight_success.emit()
             return
 
         self.export.begin_printer_preflight.emit()
@@ -680,7 +682,9 @@ class Controller(QObject):
         logger.info('Running export preflight check')
 
         if not self.qubes:
-            self.export.preflight_check_call_success.emit()
+            import threading
+            threading.Timer(3, self.export.preflight_check_call_success.emit).start()
+            # self.export.preflight_check_call_success.emit()
             return
 
         self.export.begin_preflight_check.emit()

[ninavizz]

@ntoll Random clarification...

1. All of the things in this PR, are for both the Export and Print flows, which begin on near-identical screens. Written content is different, the "Ready" state device icon is different, but that's mostly it.
2. Per our Slack DM, that first screen never gets the active spinner on its Continue/primary button, but the rest of the primary buttons in both flows do. That first screen gets a disabled "Continue" primary button.
3. I'll be around, later—SMS if u need, will be binge-watching Better Call Saul late tonite!

[sssoleileraaa]

In case it's helpful, here's how you can test each step for both print and export for both failure and success states without QubesOS:

--- a/securedrop_client/logic.py
+++ b/securedrop_client/logic.py
@@ -43,7 +43,7 @@ from securedrop_client.api_jobs.uploads import SendReplyJob, SendReplyJobError,
     SendReplyJobTimeoutError
 from securedrop_client.api_jobs.updatestar import UpdateStarJob, UpdateStarJobException
 from securedrop_client.crypto import GpgHelper
-from securedrop_client.export import Export
+from securedrop_client.export import Export, ExportError
 from securedrop_client.queue import ApiJobQueue
 from securedrop_client.sync import ApiSync
 from securedrop_client.utils import check_dir_permissions
@@ -668,7 +668,10 @@ class Controller(QObject):
         logger.info('Running printer preflight check')
 
         if not self.qubes:
-            self.export.printer_preflight_success.emit()
+            import threading
+            threading.Timer(3, lambda: self.export.printer_preflight_failure.emit(ExportError('errrrror'))).start()
+            # threading.Timer(3, self.export.printer_preflight_success.emit).start()
+            # self.export.printer_preflight_success.emit()
             return
 
         self.export.begin_printer_preflight.emit()
@@ -680,7 +683,10 @@ class Controller(QObject):
         logger.info('Running export preflight check')
 
         if not self.qubes:
-            self.export.preflight_check_call_success.emit()
+            import threading
+            threading.Timer(3, self.export.preflight_check_call_success.emit).start()
+            # threading.Timer(3, lambda: self.export.preflight_check_call_failure.emit(ExportError('errrrror'))).start()
+            # self.export.preflight_check_call_success.emit()
             return
 
         self.export.begin_preflight_check.emit()
@@ -699,7 +705,10 @@ class Controller(QObject):
             return
 
         if not self.qubes:
-            self.export.export_usb_call_success.emit()
+            import threading
+            threading.Timer(3, lambda: self.export.export_usb_call_failure.emit(ExportError('errrrror'))).start()
+            # threading.Timer(3, self.export.export_usb_call_success.emit).start()
+            # self.export.export_usb_call_success.emit()
             return
 
         self.export.begin_usb_export.emit([file_location], passphrase)

[ntoll]

OK... this was more fiddly than I imagined since the buttons were moving around the UI because the QSize was not what it ought to be and I simply had to use my Eyeball Mk.1 to judge what the actual dimensions were and figure out the CSS. These truly are artisnal, hand crafted, organic spinner buttons. 😉 Look at how hipster am I..? (I still hate CSS with a vengeance though... I must have spent years faffing about with it so it's "just so" for no logical reason.)

@creviera your input was invaluable in allowing me to see how it'd work on Qubes. Thank you. Screenies below...

Export success:

Export fail (the error message is me testing... please ignore the content):

Print success:

Print fail (behaviour is as currently is, error message is me testing):

The most important aspect of these screenies is that the spinners and other active state doodahs appear in the right place and stop at the appropriate point.

This last part was about as fiddly as the string section of the Boston Pops Orchestra playing selections from "Fiddler on the Roof". 🎻 😉

[ntoll]

OK... putting aside the build fail (due to CI infrastructure problem AFAICT), this is ready for review. Please see screenies in previous comment. Many thanks to @ninavizz and @creviera for all the help getting this over the line.

[sssoleileraaa]

I think this looks great @ntoll! I'll try to see what's going on with the functional test failure for starring... looks like we're getting sqlite3.OperationalError: unable to open database file

[redshiftzero]

that test error was a flake (cross-linking #956), restarted and CI now passes (test-buster job will appear as succeeded in ~10 mins)

[eloquence]

This is marked to resolve #928. Does it in fact fully do so, including these two items?

• Disable Password field when actively submitting pane for auths
• When returning an error'd screen, return the screen to its state w/o the error

[ntoll]

OK... here's a couple of screenies to show how this now looks in light of @eloquence's pointing out I'd missed a couple of points; disabling the password field on submit and ensuring the error message is displayed as expected and then dimmed on re-submit:

[ninavizz]

Ohhh that is lovely! <3

Mebbe dim a bit more, as the Qubes laptop has a much lower-contrast screen than our fancypants monitors do... and it'd be nice to be a touch more noticeable? I'd spec'd .4 alpha in my Zeplin, but wd be ok going down to .2, as legibility is not important when its processing.

[sssoleileraaa]

Just finished testing in Qubes and this lgtm

[ntoll]

Hurrah. Thank you for all your help folks...! 🌞