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

Query Delay after Clicking "OK" #719

Closed
umut-er opened this issue Jun 25, 2024 · 15 comments
Closed

Query Delay after Clicking "OK" #719

umut-er opened this issue Jun 25, 2024 · 15 comments
Assignees
Milestone

Comments

@umut-er
Copy link

umut-er commented Jun 25, 2024

To address the concerns in issue #709, we had to introduce additional queries after we click "OK" on the query tab. However, we currently do these additional queries before we change the display. This causes some amount of delay after the user clicks "OK", which can cause the user to believe that the click failed, prompting them to click multiple times. This results in multiple queries being sent out, which is not intended. It would be clearer if we just changed the display so as to prevent any duplicate queries.

@ugurdogrusoz
Copy link
Contributor

@NoorMuhammad1 These additional queries mentioned are not expensive (long), are they?

@ugurdogrusoz ugurdogrusoz added this to the version 4.0 milestone Jun 25, 2024
@NoorMuhammad1
Copy link
Contributor

No i think we just need to change the location where we start the spinner and toggle the menu. I think it will not be related to the queries.

@umut-er
Copy link
Author

umut-er commented Jun 25, 2024

@NoorMuhammad1 These additional queries mentioned are not expensive (long), are they?

They take as long as the display to disappear currently, give or take. I do agree that this issue is not directly related to how long those queries take.

@ugurdogrusoz
Copy link
Contributor

@NoorMuhammad1 we no longer have that problem of pause. However, I noticed that the spinner disappears and re-appears. Is this due to two separate queries? Can we remedy this?

@NoorMuhammad1
Copy link
Contributor

In order to solve the issue of user entering invalid gene name as input, i first make a request to verify that the input gene names exist in the system or not. Once we get a positive response for that, we go on to run rest of the query. So because there are 2 queries running, that is why the spinner runs two times

@ugurdogrusoz
Copy link
Contributor

That's what I thought. See if you can have a single spinner around both queries.

@NoorMuhammad1
Copy link
Contributor

I have rectified the double spinner problem, by removing the spinner from the gene symbol verification process and make a global call for spinner for each process.

@ugurdogrusoz
Copy link
Contributor

Looks like I didn't test enough. See problem below detected by @umut-er:

Screen.Recording.2024-06-27.at.23.23.36.mp4

@umut-er
Copy link
Author

umut-er commented Jun 28, 2024

Here are a couple of suggestions on what this should do:

  1. The confirmation window should appear immediately after the query window, before all queries (note that this is the behavior in public newt).
  2. The spinner should appear only after user accepts the confirmation window and the confirmation window disappears. The spinner shouldn't appear at all if the user doesn't accept the confirmation window.
  3. If the query is invalid (either gene doesn't exist or result is empty) ideally we wouldn't want to lose the initial map.

There are other ways of doing this for sure, but I think these are very good suggestions and that's why I am sharing them.

@NoorMuhammad1
Copy link
Contributor

NoorMuhammad1 commented Jun 28, 2024

I have fixed the bug related to the spinner not closing on the confirmation box cancel button. But here are some issues with the full following of the suggestions mentioned above:

  1. If the confirmation window appears right before all queries then there will be multiple spinners which is not desired.
  2. The spinner is started globally and then closed based on different responses and errors, so we want to implement the second point then we will need to start spinner multiple times and close it multiple times

We can simplify all of this, but that would need significant changes in the structure of the backbone views. If we want to preserve the current structure, then we have these issues, if not then I can restructure the views so that they are more coherant and don't have these problems.

@ugurdogrusoz
Copy link
Contributor

ugurdogrusoz commented Jun 28, 2024

What are the visible effects of current code? Can you list steps to reproduce any bug looking behavior? Let's not change the current behavior even when it's not ideal, unless it's an obvious bug.

@umut-er
Copy link
Author

umut-er commented Jun 28, 2024

I have implemented the behavior in the public newt for the neighborhood query type. It currently looks like this (not uploaded to internal server or pushed to unstable yet):

Screen.Recording.2024-06-28.at.20.40.23.mp4

Note that the confirmation window appears immediately after the user interacts with the query window and the spinner appears only after the user interacts with the confirmation window. This is consistent with the behavior in the public newt. Please do check it by yourself by trying this in the internal server and the public newt. If you like this one I can implement it for the other query types as well.

PS. Currently there is another bug. If you try to query for "$$$$" (or any combination of special characters only) you get a bug (at least in the neighborhood type query). The same is also present if you try to increase the stop distance to 10 for example. Try it yourself on the internal server please. It's a very easy fix and I have dealt with it in the version you see in the video.

@umut-er
Copy link
Author

umut-er commented Jul 2, 2024

Hey @NoorMuhammad1, since we agree that the current state of the code is good, why not assign this to @ugurdogrusoz and wait for review?

@NoorMuhammad1
Copy link
Contributor

I agree.

@ugurdogrusoz
Copy link
Contributor

Looks good @NoorMuhammad1 !

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

No branches or pull requests

3 participants