It's instroduced in 6af0dad2eeba
Details
Details
All transitions between different Dolphin views looks correct.
Diff Detail
Diff Detail
- Repository
- R318 Dolphin
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
- Search (ctrl+f)
- Open new tab
- close opened tab
It should happend if only open tab then close it.
Comment Actions
Ok then this is bug https://bugs.kde.org/show_bug.cgi?id=379135 and the regression is caused by 6af0dad2eeba.
Checking the pointer may fix some crashes but doesn't fix the underlying issue (what if the pointer is not zero but still invalid?)
Comment Actions
I don't see how commit that you linked can interact to change tab index. http://doc.qt.io/qt-5/qtabwidget.html#widget widget(index) returns nullptr or valid pointer.
Comment Actions
Yeah I don't know either why. Can you can confirm that it doesn't crash anymore if you revert this commit?
Comment Actions
I'm surprised to see that bug report before 17.04.1, i daily use searching, tabbing, splitting and i never experianced a crash before last update. I will investigate to determine why is not chacked in other places. After the patch, no crash happens.
Comment Actions
Elvis, you are right. I revert 6af0dad2eeba, it's doing in other way. Calling viewContainer->setActive(false), whenever it's called after close tab (because close tab activate DolphinTabWidget::currentTabChanged, causing recursive stack smash or reading the removed index (stack trace).
I simplified the logic by removing DolphinViewContainer::activate, the problem is on closed tab KUrlNavigator or DolphinSearchBox reactivate the view (cause it's connected through DolphinViewContainer::activate). I don't see any reason KUrlNavigator, DolphinSearchBox and DolphinView to reactive itselfs.
Comment Actions
Thanks, this is starting to look good. Please fix the inline issues.
| src/dolphinmainwindow.cpp | ||
|---|---|---|
| 926 ↗ | (On Diff #14718) | Not necessary, the new view container is already active (by definition). |
| src/dolphinviewcontainer.cpp | ||
| 73 ↗ | (On Diff #14623) | Regression: open slit view and click the url navigator of the inactive view. This should toggle the active view but it doesn't anymore. |
| 87 ↗ | (On Diff #14623) | Same regression. Open split view and open search box in both views. If you click the search box of the inactive view, the view no longer becomes active. |
| src/dolphinviewcontainer.h | ||
| 245 ↗ | (On Diff #14623) | Don't remove this slot, we still need it (see below). |
| src/dolphinviewcontainer.h | ||
|---|---|---|
| 245 ↗ | (On Diff #14623) | I'll do it in other way, this slot is *pure* evil, it should be removed |
Comment Actions
Complete removal of activate, this slot and connections to it, like KUrlNavigator or SearchBox is unwanted and makes infinite loop to view (true / false) cause when url is changed KUrlNavigator fire activate then viewchanged is fired in there view->setActive is called to false, now we enter an infinite loop.
Comment Actions
I'm not convinced. How do I reproduce the infinite loop of signals/slots? (I tried to put some debug prints but everything seems fine).
I see another regression now:
- Open split view
- Open a new tab
- Go back to the old tab
- Right click doesn't work
Comment Actions
Reproduce infinite loop:
Revert connections kurl navigator, searchbox to activate, open 2 tabs, return to first click bookmark in places (make sure you have other changes). The new regression is because focus is missing, if you click in the view, it works. I'll correct it.
Comment Actions
I *really* hate this code, but it works correct all the way. The situation is quite complicated between different tab views
Comment Actions
There is still a small regression, unfortunately:
- Open split view
- Open search box in both views
- Click the search box of the inactive view
- Palette of the inactive view doesn't change
| src/dolphinviewcontainer.cpp | ||
|---|---|---|
| 609–614 ↗ | (On Diff #14710) | Why this change of behavior? If I click the file view I don't expect to get focus in the search box. |
| src/dolphinviewcontainer.cpp | ||
|---|---|---|
| 609–614 ↗ | (On Diff #14710) | We should not focus view when searchbox is enabled or we end-up with same crash. |
| src/dolphinmainwindow.cpp | ||
|---|---|---|
| 1474–1476 ↗ | (On Diff #14718) | I still think that D5919 would be a better fix for https://bugs.kde.org/show_bug.cgi?id=379135 It is just wrong that DolphinTabWidget's widget(currentIndex()) returns nullptr if currentIndex() == 1 and count() == 2. But the rest of this patch looks good as fix for https://bugs.kde.org/show_bug.cgi?id=380032 |
| src/dolphintabpage.cpp | ||
| 298 ↗ | (On Diff #14718) | Why active instead of m_active here? (if active is false, m_active would be true if split view is disabled) |
| src/dolphintabpage.h | ||
| 133 ↗ | (On Diff #14718) | This is not a signal, so "notify" sounds wrong for this method. Let's use something like "Set whether the tab page is active". |
| src/dolphintabpage.cpp | ||
|---|---|---|
| 298 ↗ | (On Diff #14718) | If split is disabled m_active bacame true, but we want view to goes from false to true, this will not happen we will have |
| src/dolphintabpage.cpp | ||
|---|---|---|
| 298 ↗ | (On Diff #14718) | Ok, please add some comments because this could be confusing. |
| src/dolphinmainwindow.cpp | ||
|---|---|---|
| 1474–1476 ↗ | (On Diff #14718) | I saw the code in QTabWdiget, i think the problem is the quque looks like: |
| src/dolphinmainwindow.cpp | ||
|---|---|---|
| 1474–1476 ↗ | (On Diff #14718) | That's too late I think, the crash happens before removeTab() returns. |
Comment Actions
Thanks! Looks good to me now.
Please don't forget to add:
BUG: 379135
BUG: 380032
in the commit message.