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

Cannot save text opened in external application (Windows) #2402

Closed
chrisjlocke opened this issue Sep 17, 2020 · 9 comments
Closed

Cannot save text opened in external application (Windows) #2402

chrisjlocke opened this issue Sep 17, 2020 · 9 comments
Assignees
Labels
bug Confirmed bugs or reports that are very likely to be bugs. windows
Milestone

Comments

@chrisjlocke
Copy link
Member

chrisjlocke commented Sep 17, 2020

Haven't used this feature before, but it appears when in the 'Edit Database Cell' dock, there is a button to edit the text in an external application (eg, Notepad, if on Windows).
However, it wasn't possible to save this text within Notepad - the file was locked (I assume by DB4S...)

Screencast: https://streamable.com/5bbtoj

So I clicked the button, edited the file, but when you save it, Notepad can't. I saved the file elsewhere, but obviously, DB4S couldn't use this file.
I assume this will work on other OS's, that don't lock the files so strongly as Windows.
This was with the latest nightly.

@chrisjlocke chrisjlocke changed the title Cannot save text if opened in external application Cannot save text opened in external application (Windows) Sep 17, 2020
@chrisjlocke chrisjlocke added the bug Confirmed bugs or reports that are very likely to be bugs. label Sep 17, 2020
@mgrojo mgrojo self-assigned this Sep 17, 2020
@mgrojo
Copy link
Member

mgrojo commented Sep 17, 2020

Oh, you were the first to complain... it seems this new feature is not very used, or nobody cared in Windows.

Yes, in Linux it works without problem, but I've checked the code and it isn't obvious. In theory, the file is closed before calling the external application. I guess not everything is done synchronously.

Aha, so this is the problem:

Reopening a QTemporaryFile after calling close() is safe. For as long as the QTemporaryFile object itself is not destroyed, the unique temporary file will exist and be kept open internally by QTemporaryFile.

So Qt tried to outsmart me... (when I say close, it's close). Ok, I'll have to find an alternate way.

mgrojo added a commit that referenced this issue Sep 20, 2020
According to Qt documentation:
> Reopening a QTemporaryFile after calling close() is safe. For as long as
> the QTemporaryFile object itself is not destroyed, the unique temporary
> file will exist and be kept open internally by QTemporaryFile.

Keeping the file open by DB4S means that later it cannot be opened by the
external application in Windows, since it is locked.

See issue #2402
@mgrojo
Copy link
Member

mgrojo commented Sep 20, 2020

@chrisjlocke I'd figured out how to close the file before the external application is launched. I haven't tested in Windows. You know the proceedings 😉

@chrisjlocke
Copy link
Member Author

chrisjlocke commented Sep 21, 2020

Works perfectly!
Using it though, I'm wondering if the workflow could do with a tweak.
At the moment, you edit the file, save it, close the third-party application, return to DB4S, click 'Apply', and then have to click 'Apply' again (albeit a different 'Apply' button). There is no need for the 'Cancel' and 'Apply' - it can simply be an 'OK'. If you didn't want the changes, simply don't click 'Apply' when you return to DB4S - move to a different cell and the changes are lost*.
At the moment, having to click 'Apply' implies you've applied it, when you haven't - you have to click 'Apply' a further time.

*This is also a bug-bear. If there are dirty changes, it would be so nice if DB4S would say, "Hang on - you didn't click 'Apply' - you OK with losing those four hours you spent typing into that cell?' 😉

@mgrojo
Copy link
Member

mgrojo commented Sep 25, 2020

Works perfectly!
Using it though, I'm wondering if the workflow could do with a tweak.
At the moment, you edit the file, save it, close the third-party application, return to DB4S, click 'Apply', and then have to click 'Apply' again (albeit a different 'Apply' button). There is no need for the 'Cancel' and 'Apply' - it can simply be an 'OK'. If you didn't want the changes, simply don't click 'Apply' when you return to DB4S - move to a different cell and the changes are lost*.
At the moment, having to click 'Apply' implies you've applied it, when you haven't - you have to click 'Apply' a further time.

I felt, it wasn't well polished. I accept your suggestion. Is this wording acceptable?

The data has been saved to a temporary file and has been opened with the default application. 
You can now edit the file and, when you are ready, close this dialog and the data will be loaded 
into the cell editor, ready for being applied.

*This is also a bug-bear. If there are dirty changes, it would be so nice if DB4S would say, "Hang on - you didn't click 'Apply' - you OK with losing those four hours you spent typing into that cell?' 😉

I was surprised nobody seemed to care about this 😄 Yes, I agree. Then it might make sense to add a Cancel button to the cell editor, to avoid any prompting.

@mgrojo
Copy link
Member

mgrojo commented Sep 26, 2020

After having removed the Cancel and Apply buttons and replaced them by an OK button, I wonder if it make sense to have to click OK and then Apply in the Cell editor. Why not leaving the Cancel and Apply buttons in the dialog, and when clicking Apply saving the data directly in the cell. Clicking Cancel will, of course, throw the data away and not touch anything. The wording could be mostly the same as in the current revision:

"The data has been saved to a temporary file and has been opened with the default application. "
"You can now edit the file and, when you are ready, apply the saved new data to the cell or cancel any changes."

The implementation is equally simple in both case. @chrisjlocke, what do you reckon?

@chrisjlocke
Copy link
Member Author

Thanks for implementing this so quickly. I haven't followed your last message - got a bit lost with the OK's and Apply's, so will download the nightly and have a look.... 👍

@mgrojo
Copy link
Member

mgrojo commented Sep 27, 2020

Oh, I might be expressing myself very badly 😉 I did not push anything I was just asking for your opinion. Nevertheless, I think that this last proposal is the best one, so I'll push it and wait for your impression

mgrojo added a commit that referenced this issue Sep 27, 2020
And not to the cell editor. This will avoid a redundant click to Apply,
when the user has just pressed Apply instead of Cancel in the dialog.

See issue #2402
mgrojo added a commit that referenced this issue Oct 4, 2020
According to Qt documentation:
> Reopening a QTemporaryFile after calling close() is safe. For as long as
> the QTemporaryFile object itself is not destroyed, the unique temporary
> file will exist and be kept open internally by QTemporaryFile.

Keeping the file open by DB4S means that later it cannot be opened by the
external application in Windows, since it is locked.

See issue #2402
@mgrojo
Copy link
Member

mgrojo commented Oct 4, 2020

I've added d8ca11f to v3.12.x since it solved the problem and did not change any text. The improved version with the changed text goes to v3.13.

@mgrojo mgrojo added this to the 3.12.1 milestone Oct 4, 2020
@chrisjlocke
Copy link
Member Author

Thanks for the changes. Yes, this works well. Clicking 'apply' applies the changes straight into the cell. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bugs or reports that are very likely to be bugs. windows
Projects
None yet
Development

No branches or pull requests

2 participants