Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement support for async ops. #117

Merged
merged 9 commits into from
May 6, 2017
Merged

Conversation

tlwr
Copy link
Contributor

@tlwr tlwr commented Apr 21, 2017

This deals with #79 with 11 lines of code changes (and the addition of some tests).

The relevant code changes are exclusively the addition of async and await where necessary.

This should be able to be dealt with fairly quickly and I can clean up #89 separately.

Summary

  • Async/await to support asynchronous operations
  • Added more tests
  • eslint ecmaVersion changed from 6 to 8
  • Added jsdoc-babel plugin to jsdoc
  • Re-added the UI changes made by n1474335

@tlwr tlwr changed the title Implement support for async ops. WIP: Implement support for async ops. Apr 21, 2017
@tlwr tlwr changed the title WIP: Implement support for async ops. Implement support for async ops. Apr 22, 2017
@n1474335
Copy link
Member

Hi @tlwr, is there anything in the feature-async-ops branch that you want preserved or will this work override it entirely? Just wondering if we should be merging this into that branch or whether we just put it straight into master and delete feature-async-ops.

@tlwr
Copy link
Contributor Author

tlwr commented Apr 23, 2017

Very happy to have feature-async-ops deleted, this work overrides it entirely; everything worth keeping has been transferred. Thanks for confirming!

@n1474335
Copy link
Member

Great, this looks a lot cleaner!

There is a small UI-related bug introduced by these changes. When typing in the input box, auto-bake triggers as each new character is entered. This causes the setBakingStatus function to fire which disables and then re-enables the input box. This causes it to lose focus which means that you can't continue typing, even if the bake only took a fraction of a millisecond. A possible solution would be to record the cursor's position before disabling the input, then replace it after re-enabling. Seems a little hacky though. Simply refocusing on the input will probably place the cursor at the end which could be incredibly frustrating if you're trying to edit some text half way through. Any thoughts?

@tlwr
Copy link
Contributor Author

tlwr commented Apr 23, 2017

Just reproduced this, it is quite bad from a usability perspective. I believe the standard solution is to "debounce" the input changed event which I'm looking into right now.

@tlwr
Copy link
Contributor Author

tlwr commented Apr 23, 2017

Debouncing implemented for autobaking instead of just the input box being changed. This means that changing operation arguments doesn't "overtrigger" autobaking either.

@n1474335
Copy link
Member

n1474335 commented May 2, 2017

I'm afraid this still doesn't actually solve the root problem, it just delays it by 300ms. Focus is still removed from the input box when auto-bake does run. This also happens when you do a Ctrl+A to select all the input.

Debouncing is an interesting method, however I'm not a fan of introducing latency into the auto-baking process. It may be suitable if multiple changes are being made in quick succession, however it shouldn't delay the first bake - that should always be instant the moment you change an argument, the input, or anything else that triggers a bake.

Perhaps we shouldn't be disabling the input at all if it's going to cause this focus problem.

@tlwr
Copy link
Contributor Author

tlwr commented May 3, 2017

On Firefox 53 I do not get issues with focus (for arguments or the input pane) both with and without debouncing. However I understand this is a big usability concern.

In these commits I have:

  • Stopped the debouncing of autoBake
  • Stopped disabling the input pane when baking
  • Removed debounce from Utils; now that lodash is a dependency (as of camel/kebab/snake) we should use lodash.debounce in future

There is a UX consideration we should think about: when async baking is occurring and the input has in the meantime been changed, should we in some way mark the output as "dirty"? "Dirty" being that the contents of the output pane may no longer match the output that would be returned by baking again.

@n1474335 n1474335 merged commit 274e113 into gchq:master May 6, 2017
@n1474335
Copy link
Member

n1474335 commented May 6, 2017

Seems to be working well now. Marking the output as "dirty" once the input has changed is a very good idea. This is already potentially a problem when auto-bake is turned off, so I think a subtle indicator would be a welcome addition. Perhaps a slight change to the output border and a tooltip when you hover over it.

@tlwr
Copy link
Contributor Author

tlwr commented May 6, 2017

We finally finished this! I'll make a separate PR for the indicator.

@tlwr tlwr deleted the feature-async-ops-small branch May 6, 2017 13:25
BRAVO68WEB pushed a commit to BRAVO68WEB/CyberChef that referenced this pull request May 29, 2022
BRAVO68WEB pushed a commit to BRAVO68WEB/CyberChef that referenced this pull request May 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants