Confirm closing muliple tabs.
AbandonedPublic

Authored by shubham on Dec 15 2018, 9:09 AM.

Details

Summary

BUG: 380963

Test Plan

Diff Detail

Repository
R40 Kate
Lint
Lint Skipped
Unit
Unit Tests Skipped
There are a very large number of changes, so older changes are hidden. Show Older Changes
This revision now requires changes to proceed.Jan 8 2019, 7:22 PM
shubham updated this revision to Diff 49089.Jan 9 2019, 5:07 PM
shubham marked an inline comment as done.

Solved to close the current tab

shubham added inline comments.Jan 9 2019, 5:08 PM
kate/katemainwindow.cpp
1204

But still this issue is not solved by this solution.

shubham planned changes to this revision.Jan 9 2019, 5:11 PM
shubham retitled this revision from Confirm closing if multiple tabs are open. to Confirm closing muliple tabs..Jan 9 2019, 5:15 PM
dhaumann added inline comments.Jan 9 2019, 7:05 PM
kate/katemainwindow.cpp
1199

Much better would be a shorter notation where you also can use const:

const bool closedByUser = !qApp->isSavingSession();
shubham updated this revision to Diff 49179.Jan 10 2019, 5:40 PM
shubham marked an inline comment as done.

Simplify closedByUser boolean and make it constant.

shubham edited the test plan for this revision. (Show Details)Jan 10 2019, 5:41 PM
shubham updated this revision to Diff 49182.Jan 10 2019, 5:54 PM

Get the mainWindow's count

shubham marked an inline comment as done.Jan 10 2019, 5:54 PM

@dhaumann Everything works fine and as expected, I misinterpreted the original multi window problem. What's your say now?

mwolff resigned from this revision.Jan 12 2019, 12:32 PM
mwolff added a subscriber: mwolff.
mwolff added inline comments.
kate/katemainwindow.cpp
1201

introduce three more locals to make this line less long:

const bool closedByUser = !qApp->isSavingSession();
const bool numDocuments = KateApp::self()->documentManager()->documentList().count();
const bool numWindows = KateApp::self()->mainWindowsCount();
const bool askConfirmation = closedByUser && numDocuments > 1 && numWindows == 1;

if (askConfirmation) {
 ...

why is the number of windows affecting this btw? I mean when I close a window with multiple tabs, it should ask for confirmation, no? Why is this only asked when there is just one window?

Note that closing a single window is different from quitting the application.

Honestly this multiple tabs stuff is all over the place and IMO it should go into the frameworks as a sort of utility function in order to stay consistent instead of being written at application level. I'm sure everyone involved is familiar with the topic but I'll attach some of the confirmations just in case.

The thing is: if you have multiple windows, closing a window does NOT close the document. That's why windows or the tabs are not a good measure for this question. The currently proposed implementation only asks, if you'd really close documents which is the same as closing the application.

@emateli: I don't see how this functionality could be upstreamed and shared: it's more or less a single if statement already.

dhaumann added inline comments.Jan 12 2019, 7:08 PM
kate/katemainwindow.cpp
1201

+1 for using local variables to make the code more readable.

@dhaumann is everything else working as expected for you, did you tested it?

@dhaumann It's not about the lines of code but about making the dialog consistent in all the applications in a way that one cannot get it wrong. You can notice that the title, text, icons, confirmation are all different in each of them even though the dialog itself is trivial to implement. Okuar goes the extra step and even inverts the check box action. Even this patch, which is designed after the Dolphin implementation gets the title wrong.

Anyhow, sorry about hijacking this patch with this slightly off-topic discussion about the frameworks.

shubham updated this revision to Diff 49443.Jan 14 2019, 2:29 PM

Use local variables for improving readability.

shubham marked 2 inline comments as done.Jan 14 2019, 2:30 PM
shubham edited reviewers, added: cullmann; removed: mwolff.Jan 15 2019, 5:37 PM
dhaumann added inline comments.Jan 15 2019, 6:17 PM
kate/katemainwindow.cpp
1201

Strictly speaking, 'bool documentCount' implies this is a number, but it's a bool.

Better is:

bool multipleDocumnetsOpen = ...
bool isLastWindow = ...

Could you adapt again?

1207

I would have expected "Quit Kate" and "Cancel" instead of "Close Current Tab". Any opinions?

shubham added inline comments.Jan 15 2019, 6:21 PM
kate/katemainwindow.cpp
1207

Don't you want feature to close current tab?

The thing is: if you have multiple windows, closing a window does NOT close the document. That's why windows or the tabs are not a good measure for this question. The currently proposed implementation only asks, if you'd really close documents which is the same as closing the application.

@emateli: I don't see how this functionality could be upstreamed and shared: it's more or less a single if statement already.

I think the advantage to upstreaming would be less about removing code duplication and more about harmonizing the text and appearance.

shubham updated this revision to Diff 49594.Jan 16 2019, 7:47 AM

Rename bools

shubham marked an inline comment as done.Jan 16 2019, 7:48 AM
shubham updated this revision to Diff 49992.Jan 21 2019, 2:32 PM
  1. Use createMessageBox
  2. Remove "Cancel" button
shubham edited the test plan for this revision. (Show Details)Jan 21 2019, 2:33 PM

Hi ;=)

Sorry that I didn' respond earlier.

In principle I have no issues with a dialog that once tells "are you really sure" if multiple documents are open and you close the last window aka exiting the application.

But is "tab" really the right naming? We are talking about "multiple documents" everywhere.
Should it not tell

"You have multiple documents open, are you sure you want to quit?""

@cullmann Apart from naming, what do you think about the code? Is it okay?

I would say the only directly visible error is that

KXmlGuiWindow::closeEvent(e);

should be called always at the end of the function?

shubham updated this revision to Diff 50757.Feb 3 2019, 7:32 AM
  1. Correct message
  2. Call KXmlGuiWindow::closeEvent(e) at the end of function
shubham edited the test plan for this revision. (Show Details)Feb 3 2019, 7:33 AM
shubham updated this revision to Diff 50758.Feb 3 2019, 7:36 AM

Rename Close Current Tab button to Close Current Document

shubham edited the test plan for this revision. (Show Details)Feb 3 2019, 7:36 AM
loh.tar added a subscriber: loh.tar.Feb 3 2019, 9:22 AM
  1. Remove "Cancel" button

Um, why no Cancel button? As @emateli had shown this is usual. Is there a disadvantage to offer such button too?

Um, why no Cancel button? As @emateli had shown this is usual. Is there a disadvantage to offer such button too?

It is a waste of space and not useful, same can be achieved through x button. Also if dominik can remember, we had talked on IRC to remove it.

Ping? Any comments, this patch is in review queue for almost 2 months now.

I can live with this.
Have other still issues with this patch?

Perhaps some feedback by ngraham :=)

Have other still issues with this patch?

The reason/explanation to remove the Cancel button has not convinced me.
To close the question by X on the frame to cancel the process is for me unapparent. IIRC has in the past such boxes no such button to avoid any ambiguity.

The last bool askConfirmation is for my taste not needed, but not bad either.

ngraham requested changes to this revision.Feb 13 2019, 2:36 PM

There definitely needs to be a cancel button. We can't go inventing new UI styles and making this dialog inconsistent with the same dialog used in other tab-based KDE apps. Let's restore the cancel button.

This revision now requires changes to proceed.Feb 13 2019, 2:36 PM

@ngraham What about the code? Is it okay if you had a look?

shubham updated this revision to Diff 51592.Feb 13 2019, 2:46 PM

Restore Cancel button

shubham edited the test plan for this revision. (Show Details)Feb 13 2019, 2:47 PM
ngraham accepted this revision.Feb 13 2019, 6:48 PM

I'm okay with the UX for this now.

cullmann accepted this revision.Feb 15 2019, 8:01 AM

Ok with this, thanks, will apply it.
Thanks for taking into account all the feedback!

This revision was not accepted when it landed; it landed in state Needs Review.Feb 15 2019, 8:03 AM
Closed by commit R40:1fa587b0669b: Confirm closing muliple tabs. (authored by shubham, committed by cullmann). · Explain Why
This revision was automatically updated to reflect the committed changes.
mwolff added inline comments.Feb 15 2019, 10:04 PM
kate/katemainwindow.cpp
1226

this setting isn't persisted anywhere, so now we get this annoying question every time we close kate with more than one document... did you even test this at all?

How to take settings into account?

cullmann reopened this revision.Feb 16 2019, 3:31 PM

:( Nope, I never tested that checkbox.
But actually, if it would work, I must confess if you once selected "close this document", you will never be able to close Kate with just pressing the windows "x".

cullmann requested changes to this revision.Feb 16 2019, 3:36 PM

Actually, to fix this properly, one would need some entry in the config dialog, too, to reset that change.
And I think that is overkill, I would like that we just abandon this and the related bug report.
The current state was ok >> 10 years, the increase in "usability" is IMHO not worth the amount of complexity we need to introduce.

This revision now requires changes to proceed.Feb 16 2019, 3:36 PM

@cullmann Don't you think this is some nice feature to have, having think of frustration one can have when they close kate thinking it will only close current tab, and unexpectedly all their tabs are closed.

Personally, I would directly click once "don't ask me ever again" and say "close the application".

And you will get then the same issues again, as you will never be asked again.
And if you want to re-decide, you need to search for some setting.

If you look at other editors, I tried gedit, I don't see that behavior, too.

Yeah, this confirmation dialog isn't my first choice either.

What I would strongly prefer--and what many other nice text editors today also do-- is to always save the current tabs on quit, so if you quit by accident you automatically get back all your active tabs just by re-opening the app, without having to do any setup or configuration beforehand.

Kate already supports sessions which can do this, so maybe if we step back, the solution is to simply create a new session by default, as requested in https://bugs.kde.org/show_bug.cgi?id=402598. If we do something like that, we should revert this. And maybe we should revert it anyway given that it's buggy and the UX isn't great.

Ok, finally we all seem to agree on my concerns. Personally, I think we should reject this feature and close the associated bug as won't-fix with a nice explanation that we tried but it does not really work... Not all wishes from users make sense, we simply cannot please everyone.

+1 on reverting this, but I think we should put some thought into fixing the issue motivating the desire to do this: the fact that without manually invoking Kate's session-saving features, closing a window with multiple tabs in it results in that tab history being lost. I really think we should consider creating a session by default so tab history is remembered unless the user explicitly opts out of it.

shubham abandoned this revision.Feb 17 2019, 4:40 PM

I think this is going towards being abandoned.

+1 on reverting this, but I think we should put some thought into fixing the issue motivating the desire to do this: the fact that without manually invoking Kate's session-saving features, closing a window with multiple tabs in it results in that tab history being lost. I really think we should consider creating a session by default so tab history is remembered unless the user explicitly opts out of it.

+1 I always configure all apps to act like that - it should be the default!