Changeset View
Standalone View
kate/katemainwindow.cpp
Show All 26 Lines | |||||
27 | #include "katepluginmanager.h" | 27 | #include "katepluginmanager.h" | ||
28 | #include "kateconfigplugindialogpage.h" | 28 | #include "kateconfigplugindialogpage.h" | ||
29 | #include "kateviewmanager.h" | 29 | #include "kateviewmanager.h" | ||
30 | #include "kateapp.h" | 30 | #include "kateapp.h" | ||
31 | #include "katesavemodifieddialog.h" | 31 | #include "katesavemodifieddialog.h" | ||
32 | #include "katemwmodonhddialog.h" | 32 | #include "katemwmodonhddialog.h" | ||
33 | #include "katesessionsaction.h" | 33 | #include "katesessionsaction.h" | ||
34 | #include "katesessionmanager.h" | 34 | #include "katesessionmanager.h" | ||
35 | #include "katetabbar.h" | ||||
35 | #include "kateviewspace.h" | 36 | #include "kateviewspace.h" | ||
36 | #include "katequickopen.h" | 37 | #include "katequickopen.h" | ||
37 | #include "kateupdatedisabler.h" | 38 | #include "kateupdatedisabler.h" | ||
38 | #include "katedebug.h" | 39 | #include "katedebug.h" | ||
39 | #include "katecolorschemechooser.h" | 40 | #include "katecolorschemechooser.h" | ||
40 | #include "katefileactions.h" | 41 | #include "katefileactions.h" | ||
41 | 42 | | |||
42 | #include <KActionMenu> | 43 | #include <KActionMenu> | ||
▲ Show 20 Lines • Show All 1143 Lines • ▼ Show 20 Line(s) | 1185 | case Qt::ForwardButton: | |||
1186 | break; | 1187 | break; | ||
1187 | case Qt::BackButton: | 1188 | case Qt::BackButton: | ||
1188 | slotFocusPrevTab(); | 1189 | slotFocusPrevTab(); | ||
1189 | break; | 1190 | break; | ||
1190 | default: ; | 1191 | default: ; | ||
1191 | } | 1192 | } | ||
1192 | } | 1193 | } | ||
1193 | 1194 | | |||
1195 | void KateMainWindow::closeEvent(QCloseEvent* event) | ||||
1196 | { | ||||
1197 | // Find out if Kate is closed directly by the user or | ||||
1198 | // by the session manager because the session is closed | ||||
1199 | bool closedByUser = true; | ||||
dhaumann: Much better would be a shorter notation where you also can use const:
const bool… | |||||
1200 | if (qApp->isSavingSession()) { | ||||
1201 | closedByUser = false; | ||||
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… | |||||
1202 | } | ||||
1203 | | ||||
1204 | KateTabBar *tab = new KateTabBar(window()); | ||||
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. | |||||
1205 | if (tab->count() > 1 && closedByUser) { | ||||
1206 | KGuiItem yes(i18nc("@action:button 'Quit Kate' button", "&Quit %1", QGuiApplication::applicationDisplayName()), | ||||
1207 | QIcon::fromTheme(QStringLiteral("application-exit"))); | ||||
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? | |||||
1208 | KGuiItem no(i18n("C&lose Current Tab"), QIcon::fromTheme(QStringLiteral("tab-close"))); | ||||
1209 | | ||||
1210 | const auto result = KMessageBox::warningYesNoCancel(nullptr, | ||||
1211 | i18n("You have multiple tabs open in this window, are you sure you want to quit?"), | ||||
1212 | i18n("Confirmation"), | ||||
1213 | yes, | ||||
1214 | no, | ||||
1215 | KStandardGuiItem::cancel(), | ||||
1216 | i18n("Do not ask again")); | ||||
1217 | | ||||
1218 | switch (result) { | ||||
1219 | case KMessageBox::Yes: | ||||
1220 | break; | ||||
1221 | 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… | |||||
1222 | // fub=nctionality for closing current tab | ||||
1223 | Q_FALLTHROUGH(); | ||||
1224 | default: | ||||
1225 | event->ignore(); | ||||
1226 | 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… | |||||
1227 | } | ||||
1228 | } | ||||
1229 | | ||||
1230 | KXmlGuiWindow::closeEvent(event); | ||||
1231 | } | ||||
1232 | | ||||
1194 | void KateMainWindow::slotFocusPrevTab() | 1233 | void KateMainWindow::slotFocusPrevTab() | ||
1195 | { | 1234 | { | ||
1196 | if (m_viewManager->activeViewSpace()) { | 1235 | if (m_viewManager->activeViewSpace()) { | ||
1197 | m_viewManager->activeViewSpace()->focusPrevTab(); | 1236 | m_viewManager->activeViewSpace()->focusPrevTab(); | ||
1198 | } | 1237 | } | ||
1199 | } | 1238 | } | ||
1200 | 1239 | | |||
1201 | void KateMainWindow::slotFocusNextTab() | 1240 | 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: