BUG: 380963
Details
Details
Diff Detail
Diff Detail
- Repository
- R40 Kate
- Branch
- arcpatch-D17599
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 7373 Build 7391: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
| kate/katemainwindow.cpp | ||
|---|---|---|
| 1203 | But still this issue is not solved by this solution. | |
| kate/katemainwindow.cpp | ||
|---|---|---|
| 1198 | Much better would be a shorter notation where you also can use const: const bool closedByUser = !qApp->isSavingSession(); | |
Comment Actions
@dhaumann Everything works fine and as expected, I misinterpreted the original multi window problem. What's your say now?
| kate/katemainwindow.cpp | ||
|---|---|---|
| 1200 | 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. | |
Comment Actions
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.
Comment Actions
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.
| kate/katemainwindow.cpp | ||
|---|---|---|
| 1200 | +1 for using local variables to make the code more readable. | |
Comment Actions
@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.
| kate/katemainwindow.cpp | ||
|---|---|---|
| 1206 | Don't you want feature to close current tab? | |
Comment Actions
I think the advantage to upstreaming would be less about removing code duplication and more about harmonizing the text and appearance.
Comment Actions
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?""
Comment Actions
I would say the only directly visible error is that
KXmlGuiWindow::closeEvent(e);
should be called always at the end of the function?
Comment Actions
Um, why no Cancel button? As @emateli had shown this is usual. Is there a disadvantage to offer such button too?
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
Ok with this, thanks, will apply it.
Thanks for taking into account all the feedback!
| kate/katemainwindow.cpp | ||
|---|---|---|
| 1225 | 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? | |
Comment Actions
:( 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".
Comment Actions
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.
Comment Actions
@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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
+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.