Skip to content

Conversation

@taooceros
Copy link
Member

@taooceros taooceros commented Jan 15, 2021

@jjw24
Copy link
Member

jjw24 commented Jan 17, 2021

what happens when the query is still loading though? like if its a long running query, i click on some other window and come back a minute later, this change means the bar will stop if it's hidden despite the query is still running right?

@taooceros
Copy link
Member Author

hmmm you are right. Let me use the dispatcher to disable and restart animation only when Progressbar visibility change instead of Mainwindow visibility.

@jjw24
Copy link
Member

jjw24 commented Jan 20, 2021

i dont think this is working as intended. Visibility doesnt seem to be wired up properly, because regardless whether the mainwindow is hidden or visible, the visibility is always visible. Same for progress bar visibility as well, it's always visible == true

{
Dispatcher.Invoke(() =>
{
if (ProgressBar.Visibility == Visibility.Hidden)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldnt this be MainViewModel.MainWindowVisibility? i thought we are resuming and pausing progress bar animation based on the visibility of mainwindow

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens when the query is still loading though? like if its a long running query, i click on some other window and come back a minute later, this change means the bar will stop if it's hidden despite the query is still running right?

That's the reason changing this to progressbar visibility

1. Disable animation when progressbar is hidden or mainwindow is collapsed
2. resume only when both visibility and progressbar visibility is visible
@taooceros
Copy link
Member Author

Done, please review.

@taooceros taooceros requested a review from jjw24 January 22, 2021 09:14
}
else
{
_progressBarStoryboard.Pause();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to be called everytime the mainwindow is hidden, how come a check is not added here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in db63833

@jjw24 jjw24 added the enhancement New feature or request label Jan 24, 2021
@jjw24 jjw24 linked an issue Jan 24, 2021 that may be closed by this pull request
@taooceros
Copy link
Member Author

taooceros commented Jan 24, 2021

Not sure whether this can fix #128. Let's do more testing before close that via this pr.

@jjw24
Copy link
Member

jjw24 commented Jan 24, 2021

Although not from this change, this code does not set the progress bar when the main window is hidden. To repro, when main window is visible click anywhere else on the screen, the main window will be hidden, but the Visibility property is still visible rather than hidden.

Wonder if this is an easy fix we can do?

if (e.PropertyName == nameof(MainViewModel.MainWindowVisibility))
{
if (Visibility == Visibility.Visible)
{

@taooceros
Copy link
Member Author

Although not from this change, this code does not set the progress bar when the main window is hidden. To repro, when main window is visible click anywhere else on the screen, the main window will be hidden, but the Visibility property is still visible rather than hidden.

Wonder if this is an easy fix we can do?

if (e.PropertyName == nameof(MainViewModel.MainWindowVisibility))
{
if (Visibility == Visibility.Visible)
{

We can use MainViewModel.MainWindowVisibility instead.

@jjw24
Copy link
Member

jjw24 commented Jan 24, 2021

Not sure whether this can fix #128. Let's do more testing before close that via this pr.

oh ok gotcha

@jjw24
Copy link
Member

jjw24 commented Jan 24, 2021

👍 👍

@jjw24 jjw24 merged commit fc78c0c into Flow-Launcher:dev Jan 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

High CPU Usage

2 participants