Changeset View
Standalone View
kate/katemainwindow.cpp
Show First 20 Lines • Show All 1185 Lines • ▼ Show 20 Line(s) | 1184 | case Qt::ForwardButton: | |||
---|---|---|---|---|---|
1186 | break; | 1186 | break; | ||
1187 | case Qt::BackButton: | 1187 | case Qt::BackButton: | ||
1188 | slotFocusPrevTab(); | 1188 | slotFocusPrevTab(); | ||
1189 | break; | 1189 | break; | ||
1190 | default: ; | 1190 | default: ; | ||
1191 | } | 1191 | } | ||
1192 | } | 1192 | } | ||
1193 | 1193 | | |||
1194 | void KateMainWindow::closeEvent(QCloseEvent *e) | ||||
1195 | { | ||||
1196 | // Find out if Kate is closed directly by the user or | ||||
1197 | // by the session manager because the session is closed | ||||
1198 | const bool closedByUser = !qApp->isSavingSession(); | ||||
dhaumann: Much better would be a shorter notation where you also can use const:
const bool… | |||||
1199 | const bool multipleDocumentsOpen = KateApp::self()->documentManager()->documentList().count() > 1; | ||||
1200 | const bool isLastWindow = KateApp::self()->mainWindowsCount() == 1; | ||||
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. mwolff: introduce three more locals to make this line less long:
```
const bool closedByUser = !qApp… | |||||
dhaumann: +1 for using local variables to make the code more readable. | |||||
Strictly speaking, 'bool documentCount' implies this is a number, but it's a bool. Better is: bool multipleDocumnetsOpen = ... bool isLastWindow = ... Could you adapt again? dhaumann: Strictly speaking, 'bool documentCount' implies this is a number, but it's a bool.
Better is… | |||||
1201 | const bool askConfirmation = closedByUser && multipleDocumentsOpen && isLastWindow; | ||||
1202 | | ||||
1203 | if (askConfirmation) { | ||||
This is wrong: we should not create new GUI widgets in a close event. Simply ask the Document manager (KateDocManager?) how many documents are open. You can find plenty of uses of the document manager in the code. dhaumann: This is wrong: we should not create new GUI widgets in a close event.
Simply ask the Document… | |||||
Well, now that you found KateApp::self(), the next step is also to first check whether the MainWindow is the last one to close, since we support multiple mainwindows (View > New Window). That means, you have to also check: && KateApp::self()->mainWindowsCount() == 1 Otherwise the message box is raised even though the application is not shutting down. dhaumann: Well, now that you found KateApp::self(), the next step is also to first check whether the… | |||||
shubham: But still this issue is not solved by this solution. | |||||
1204 | KGuiItem yes(i18nc("@action:button 'Quit Kate' button", "&Quit %1", QGuiApplication::applicationDisplayName()), | ||||
1205 | QIcon::fromTheme(QStringLiteral("application-exit"))); | ||||
1206 | KGuiItem no(i18n("C&lose Current Tab"), QIcon::fromTheme(QStringLiteral("tab-close"))); | ||||
I would have expected "Quit Kate" and "Cancel" instead of "Close Current Tab". Any opinions? dhaumann: I would have expected "Quit Kate" and "Cancel" instead of "Close Current Tab". Any opinions? | |||||
shubham: Don't you want feature to close current tab? | |||||
1207 | | ||||
1208 | const auto result = KMessageBox::warningYesNoCancel(window(), | ||||
1209 | i18n("You have multiple tabs open in this window, are you sure you want to quit?"), | ||||
1210 | i18n("Confirm Closing"), | ||||
1211 | yes, | ||||
1212 | no, | ||||
1213 | KStandardGuiItem::cancel(), | ||||
1214 | i18n("Do not ask again")); | ||||
1215 | | ||||
1216 | auto view = viewManager()->activeView(); | ||||
1217 | switch (result) { | ||||
1218 | case KMessageBox::Yes: | ||||
1219 | break; | ||||
1220 | case KMessageBox::No: | ||||
how do I get the currently active KTextEditor::Document to pass to the closeDocument(). shubham: how do I get the currently active KTextEditor::Document to pass to the closeDocument(). | |||||
Please read a bit more in the code: KateMainWindow has a viewManager(), and this has a activeView(). Since you are currently in the KateMainWindow, you can simply write auto view = viewManager()->activeView(); and you have the active KTextEditor::View, which will give you the document etc. dhaumann: Please read a bit more in the code: KateMainWindow has a viewManager(), and this has a… | |||||
1221 | KateApp::self()->documentManager()->closeDocument(view->document()); | ||||
1222 | Q_FALLTHROUGH(); | ||||
1223 | default: | ||||
1224 | e->ignore(); | ||||
1225 | return; | ||||
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? mwolff: this setting isn't persisted anywhere, so now we get this annoying question every time we close… | |||||
1226 | } | ||||
1227 | } | ||||
1228 | KXmlGuiWindow::closeEvent(e); | ||||
1229 | } | ||||
1230 | | ||||
1194 | void KateMainWindow::slotFocusPrevTab() | 1231 | void KateMainWindow::slotFocusPrevTab() | ||
1195 | { | 1232 | { | ||
1196 | if (m_viewManager->activeViewSpace()) { | 1233 | if (m_viewManager->activeViewSpace()) { | ||
1197 | m_viewManager->activeViewSpace()->focusPrevTab(); | 1234 | m_viewManager->activeViewSpace()->focusPrevTab(); | ||
1198 | } | 1235 | } | ||
1199 | } | 1236 | } | ||
1200 | 1237 | | |||
1201 | void KateMainWindow::slotFocusNextTab() | 1238 | void KateMainWindow::slotFocusNextTab() | ||
▲ Show 20 Lines • Show All 60 Lines • Show Last 20 Lines |
Much better would be a shorter notation where you also can use const: