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

Close all jobs in the window when window is closed #83

Closed
bertfrees opened this issue Feb 27, 2023 · 17 comments
Closed

Close all jobs in the window when window is closed #83

bertfrees opened this issue Feb 27, 2023 · 17 comments
Milestone

Comments

@bertfrees
Copy link
Member

Closing the window (with ⌘W or the close button in the title bar) only appears to hide the window (or has the effect of hiding). When the window opens again upon clicking the tray icon (#82) the same tabs that were present when the window was closed are present. With one difference: a job form that the user has started filling in but has not been submitted yet doesn't appear the same: the title is the same but the contents are reset to the initial job form with the script drop-down list.

I think it would be better to actually close all the jobs in the window (as if the "Delete job"/"Cancel job" buttons were clicked for all jobs). For job forms that are not submitted yet, this should result in warnings: see #29. Warnings could also be shown for jobs that are completed: see #81.

It should probably not be possible to close a job that is still running because the engine does not support canceling a running job. There could be an option to force close the job though, with a warning that the job will not be deleted from the engine until it is finished, so it will keep taking up resources in the background. On the other hand, being able to close the tab of a running job and recover it later is something that is also described in our design document. However this requires a way to recover it, e.g. with a job list in the menus, and/or with the help of a notification system.

To hide the window there is the "Hide DAISY Pipeline (⌘H)" menu item.

@NPavie
Copy link
Collaborator

NPavie commented Feb 28, 2023

You say it is hiding the window : do you mean the app window stays in the dock and remain selectable with the cmd+tab selector ?
I don't see this behavior in the current dev version and have different behavior between hiding and closing in my tests but we might have changed stuff after the last release.

If you can still see the window and select it when requestion the window to close, this is a bug that might have been fixed.
If the window does not appear neither on the dock nor can be selected using cmd+tab, that mean the window has been destroyed and is not hidden.
On reopening the app, the states are currently reload from ram (including job states for monitoring) to speed up the app relaunch.

I think it would be better to actually close all the jobs in the window (as if the "Delete job"/"Cancel job" buttons were clicked for all jobs).

That seems like a regression in my own use cases but it can be done if requested.

@bertfrees
Copy link
Member Author

do you mean the app window stays in the dock and remain selectable with the cmd+tab selector?

No, it's not actually hiding the window as in what happens when you select "Hide DAISY Pipeline (⌘H)". The app is not in the dock nor in the ⌘TAB selector. But it's basically also hiding (except for the "one difference" explained above).

What I want to say with this issue is in my second paragraph: I expect all job tabs to be closed upon closing the window.

I believe this would benefit the user experience. There are apps that basically just hide when you close the window, and other apps that don't, and I guess you can debate about what is better but I think the latter is more intuitive and it is the category of apps where Pipeline belongs most.

Another thing that should be taken into account for the UX is that if you have a lot of job tabs and you can not close them all at once by closing the window, there should at least be a fast way to close individual job tabs. #79 should normally help with this: it will allow the user to do ⌘D repeatedly to close all tabs. Another thing we will want to do in the future, when we have persistence, is to save or discard a number of jobs at once, so that we don't have to choose between "save" and "discard" for every job tab that we close. If closing a window would close all tabs in it, we won't need special commands for this.

It's a good development that states are saved and reloaded. By no means I want to revert that. If not already useful now, it will certainly become useful and increase performance when we introduce persistence.

That seems like a regression in my own use cases

Why would it be a regression for you? You can still hide the window with ⌘H or by minimizing it.

@marisademeglio marisademeglio added this to the 1.0 milestone Mar 1, 2023
@NPavie
Copy link
Collaborator

NPavie commented Mar 1, 2023

You can still hide the window with ⌘H or by minimizing it.

Closing the window and minimizing a window has not the same effect on the OS side :

If i close the window, it is not displayed anymore anywhere on my desktop and in my task bar, and it is not selectable by alt+tab (there is no window registered for the system and the user to use or interact with).
I sometime want to close the window without losing the current state of the application or without completely stopping it (like outlook, teams, skype, onedrive etc in my own case).
If i minimize, the window is still present in the dock/task bar and in the alt-tab list of windows.

I think you are mangling the "window" and "application state" notions here : a window can be closed (in our case it is "destroyed" in electron terms) or hidden (meaning minimized in electron and MS Windows, but also in MacOS in my tests) while the underlying app (or local/remote server) managing the states is still running (that is : the application state is hidden). Using the "hidden" terminology for the window here is confusing and is not following neither MacOS or Windows definition.

Why would it be a regression for you?

It would be a regression because in my usage i (currently) do reopen the app do edit/relaunch existing jobs (as i'm using the app for file correction using the dtbook validator and relaunching conversions to help transcriptors).
Also, i see some possible issues with the systematic deletion of all jobs when closing the window, especially if you intend to extend the app for server-side usage.

The idea of an alert dialog asking confirmation to the user if he wants to delete all finished jobs on closing (with the option to save its choice in settings) seems the best option for both use cases (keeping the jobs state for my use case or deleting all jobs for yours).
Also regarding #79 and this issue, i wonder if a button in UI or a keyboard shortcut (like ctrlOrCmd+shift+D) to close all completed or empty jobs could not be added as intermediate step for this.

@bertfrees
Copy link
Member Author

I think you are mangling the "window" and "application state" notions here [...]

Sorry for the confusion caused by using the term "hidden". That certainly wasn't my intention. Again, the point of this issue is really about what should happen with job tabs when the window is closed using the close button or the "Close window" menu item.

I'm aware of the subtle differences between minimizing, hiding or "destroying" the window. I'm not saying these are all equivalent. I'm only saying that there are multiple alternatives for doing more or less the same thing. Minimizing and hiding were invented exactly for getting windows out of your way. These indeed don't get the window completely out of the way including from the dock and "alt-tab" list, like some apps do when you close the window, but not all apps do that. It's not something you can rely on. Some apps just hide the window. Some apps destroy the window and all the state within it, but keep the app running and active within the dock and alt-tab list. Some apps destroy the window and quit the application. It's fine that there are these kind of differences between apps because each app is just different.

For Pipeline, I have a preference for "pushing" towards closing jobs tabs whenever there is an opportunity (like when a window is closed), rather than letting the user close jobs manually. Let me try to explain a bit better why: Jobs are volatile things, especially in a desktop configuration. A user runs a conversion in order to get results (which are saved automatically), and after this the job is normally not useful anymore. When we introduce persistence there will be the possibility to store jobs for whatever reason there might be, but the default will remain that jobs are discarded when tabs are closed (after a warning). Note that it is a different situation in a server-client configuration like the web UI, where jobs are stored by default and need to be deleted manually because you first need to download the results, and jobs may be accessible by multiple people.

Also remember that it has always be the plan to have a notification system. So it will be possible to close a job tab while the job is still running and you will be notified when it is finished and the tab can be recovered.

The example that I usually compare with is Firefox, because it also has tabs, of which you can have many, and the tabs contain something volatile. (Admittedly, there is one other thing about Firefox, that the Pipeline UI currently does not have (yet? see #37), which makes it logical to close all tabs within a window when the window is closed, and that is that there can be multiple windows. It does not really make sense to save the tabs within the closed window if it is not the only window.)

@NPavie
Copy link
Collaborator

NPavie commented Mar 1, 2023

Given the discussion, i'll close this issue as it is actually a duplicate of #36 and not really a bug about the window closing mecanism that would hide the window instead :
The empty or completed jobs will be deleted on closing the window, but i'll keep a setting to allow users to keep the current behavior if they want.

@NPavie NPavie closed this as completed Mar 1, 2023
@bertfrees
Copy link
Member Author

Wait, this going a bit fast for me. Can you explain how exactly #36 has solved this? #36 seems a completely different issue to me. A setting to specify the behavior sounds good, but what will be the exact behavior now when you close the window? I want to understand the details.

@NPavie
Copy link
Collaborator

NPavie commented Mar 2, 2023

As you said in your remarks: you think the windows is hidden because the jobs state is restored when creating back the window (and as i said, this is not making the window hidden, the window is closed/destroyed but opening it back triggers a state reloading, so this issue is not about closing vs hiding but rather if the jobs state is kept or not).

So completing #36 by removing all jobs from the server (and from the app states) when closing the windows will resolve this.

@bertfrees bertfrees changed the title Closing window only hides window Close all jobs in the window when window is closed Mar 2, 2023
@bertfrees
Copy link
Member Author

OK, it thought it was clearer to have two separate issues because "remove jobs on server side when job is closed in UI" and "close jobs when window is closed" are two separate things IMO. However if you say that you will also do the latter as part of #36, it's fine too :-)

So to conclude, what exactly will the behavior be when I close a window with different kinds of jobs (not submitted, running, finished) in it? The details are important. I want to know now rather than when I test it because there is no testing round anymore before 1.0, and I think we should either do it properly or wait with it.

@bertfrees
Copy link
Member Author

bertfrees commented Mar 4, 2023

So to conclude, what exactly will the behavior be when I close a window with different kinds of jobs (not submitted, running, finished) in it?

I tested it. The behavior is as follows:

  • unsubmitted job: results in a "Some unsubmitted jobs are present", after which the job is discarded
  • running job: not discarded
  • finished job: not discarded

In other words, current not all jobs are closed when the window is closed.

  • When finished jobs are present I think they should be discarded after presenting a message saying that they will be.

  • When running jobs are present (and with the setting "close all non-running jobs"), I think the window should either:

    • a. (refuse to close)
    • b. close after a message saying that the job will be discarded (really, even though it will still be present on the server)
    • c. close after a message saying that the jobs will keep running, and include the job tabs when the window is opened again
    • d. close after a message saying that the jobs will keep running, but don't include the job tabs when the window is opened again

    In case of #​b the "close all non-running jobs" setting should of course also be changed to "close all jobs".

    In case of both #​c and #​d it should be possible to get the job back somehow. In case of #​c this amounts to getting the window back. But note that currently this is only possible using "New job" from the tray menu (remember that this is still with the setting "close all non-running jobs"). In case of #​d the job would have to be selectable from a menu (or through the planned notification system).

@marisademeglio
Copy link
Member

I find it so confusing to have the state of the window change from when I close it to the tray to when I open it back up. Especially that the way in which the state changes depends on the status of the jobs. What is the advantage of closing jobs when the window is closed to the tray?

@bertfrees
Copy link
Member Author

I find it so confusing to have the state of the window change from when I close it to the tray to when I open it back up. Especially that the way in which the state changes depends on the status of the jobs.

Exactly. This has made the UI less intuitive because we did things in a haste and didn't talk the details through. It is what I was afraid would happen, what I meant with "we should either do it properly or wait". This is not an efficient way of working.

Indeed, the initial idea of this issue was to close all tabs in the window so that when re-opened, there would be no jobs at all. Later it was decided to have a setting to control whether we want to have this behavior or if we want to get exactly the same job tabs upon re-opening the window. Anything in between (some jobs kept and some not) is indeed confusing. (Actually this makes option #​c above also a bad idea.)

What is the advantage of closing jobs when the window is closed to the tray?

As we said, both options have their advantages and users may prefer one or the other, so Nico came up with the setting. The advantage of closing jobs, the way I see it, I have tried to explain in this thread (#83 (comment) and #83 (comment)). Have you read it?

I know I don't always manage to get my ideas across perfectly. That's because some things are just hard to explain/understand, because they are based on a feeling or are so complex because of the many subtleties that need to be taken into account. I spend a lot of time trying to write down my ideas very carefully, and my comments tend to get long quickly, so sometimes choose to not overdo it. I wish we would keep discussing things until they are clear though, if needed in a call.

Anyway, since you asked perhaps I can throw in another thing to think about: If you expect that when the window is closed and re-opened the jobs tabs that were present are still present, you may expect the same when the application is quit and restarted. Because real persistence (that is, persistence across runs of the program) is not implemented today, this is not the case of course. But as you know the idea is that proper persistence will be added in the future. Now what should happen when the application is quit and restarted? Should all jobs that were present still be present? This is not possible unless the UI remembers which were the jobs tabs. (The engine remembers all saved jobs, also the ones without a job tab.) This in turn is not such a good idea because any state outside of the engine makes things more complex, and moreover the UI can in theory connect to any server, not only the embedded one. So should all unsaved jobs be discarded? This seems to be the only sensible option. It means that at exit time you need to ask the user which jobs should be saved and which should be discarded. So why not do this already as soon as the window is closed? I'm not saying it's necessarily better if we do, but it's something to think about.

@marisademeglio
Copy link
Member

I have read the issue threads, but as I'm not working on this particular one, I've stayed out of it. I suppose me getting involved at this point won't make it resolve faster, likely the opposite, but here we are 🙃

As a user, I would expect that if I keep it running in the tray, then the state persists with all jobs, whether they are finished, running, not yet submitted. And if I quit the app, the state does not persist at all.

If in the future we are supporting full persistence or connecting to a remote instance of the engine, then we'll need some more nuanced settings. I don't think we need that now though.

We will find a balance between having very thorough discussions and finishing on a tight schedule!

@bertfrees
Copy link
Member Author

bertfrees commented Mar 5, 2023

It doesn't need to be resolved fast (this wasn't an urgent issue). It needs to be well thought through, there needs to be a vision. So your opinion is needed.

@NPavie
Copy link
Collaborator

NPavie commented Mar 6, 2023

  • unsubmitted job: results in a "Some unsubmitted jobs are present", after which the job is discarded

This is on purpose (as we wanted users to get warned on deleting unsubmitted - part of form filled without being submitted - jobs).

  • running job: not discarded

This is needed for background monitoring (required for notifications and also to move the results to the user folder when finished)

  • finished job: not discarded

That's strange, the finished jobs were discarded in my tests, I might have missed a case.

In other words, current not all jobs are closed when the window is closed.

As mentioned, some jobs need to remains for background monitoring (if the user decide to wait for the engine to complete, but i added a warning )

I'll reopen if you want to continue discussion and add more precision of expected behavior here.

@NPavie NPavie reopened this Mar 6, 2023
@marisademeglio marisademeglio removed this from the 1.0-RC milestone Mar 6, 2023
@bertfrees
Copy link
Member Author

  • unsubmitted job: results in a "Some unsubmitted jobs are present", after which the job is discarded

This is on purpose

Yes, it's good! :-) The problem is the two next items.

One other thing I forgot to mention is that unsubmitted jobs are only discarded when a script has been selected (or if there was at least one other unsubmitted job with a script selected, see below).

  • running job: not discarded

This is needed for background monitoring (required for notifications and also to move the results to the user folder when finished)

Well, figuring out what to do with running jobs is indeed the most challenging part, the part that requires most thinking. This is also what I said in my initial comment. But simply including the tab on reopening the window (without some warning) is not a good solution. In my comment above I listed the ways I can think of to deal with this situation. Did you read it?

What do you mean by "this is needed [for background monitoring]"? Do you mean it is needed due to the way it is currently implemented? Any of my options #​a to #​d should be theoretically possible to implement, right?

Option #​c is probably the most sensible option given the fact that we don't have a way of bringing job tabs back at the moment. #​d is the ideal solution in the long term. #​a to #​b are less ideal.

  • finished job: not discarded

That's strange, the finished jobs were discarded in my tests, I might have missed a case.

I just discovered that they are discarded when there was also at least one unsubmitted job with a script selected. In this case the finished job is discarded, otherwise not. I can't verify whether there would be warning message for finished jobs (and am not sure if there should be).

As mentioned, some jobs need to remains for background monitoring (if the user decide to wait for the engine to complete, but i added a warning )

Do you mean a commit that adds a warning will follow? Would this be option #​c in my comment above?

@NPavie
Copy link
Collaborator

NPavie commented Mar 9, 2023

I just realized that

  • I forgot to change the text in the warning saying that some jobs are still running when you try to close the application.
    (Only the title was right)
  • I forgot the global discard case on closing ...

NPavie added a commit that referenced this issue Mar 9, 2023
NPavie added a commit that referenced this issue Mar 9, 2023
Updated the text of the warning raised when jobs are still running when
trying to close the application.
@marisademeglio marisademeglio added this to the 1.0 milestone Mar 13, 2023
@bertfrees
Copy link
Member Author

bertfrees commented Mar 16, 2023

The new behavior is as follows:

  • with "Keep all jobs opened [on closing the app window]":

    • no tabs discarded
  • with "Close all non-running jobs [on closing the app window]"

    • empty jobs: discarded
    • unsubmitted jobs: discarded after "Some unsubmitted jobs are present" warning
    • running job: not discarded, no warning
    • finished job: discarded without warning
  • with "Close the application [on closing the app window]"

The behavior is fine for empty jobs and unsubmitted jobs. It's also fine that running jobs are not discarded, although a warning (that they will keep running) seems appropriate (with a "don't warn me again" option) (I also suggested this in #83 (comment)).

Issue #125 is obviously some bug that needs to be fixed.

Discarding finished jobs is also fine. However because this does not generate a warning, people might be surprised that the job is gone when they bring back the window from the tray (with the "Close all non-running jobs" setting) or that the application can be accidentally quit without warning. Perhaps this is why Rachana complained.

My suggestion for these two problems is the following:

By reasoning about these issues it becomes more and more clear to me that it would make more sense to merge the two settings related to window closing into one.

Sorry for yet another long post :-)

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