Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[81_11] Fix the bug causing crashes when dragging tabs. #2147

Merged
merged 10 commits into from
Oct 10, 2024

Conversation

Minzihao
Copy link
Contributor

@Minzihao Minzihao commented Oct 8, 2024

How to test your changes?

Just drag it. I believe it will be fine this time.

@da-liii da-liii requested a review from Oyyko October 8, 2024 15:49
@da-liii da-liii changed the title Fix the bug causing crashes when dragging tabs. [81_11] Fix the bug causing crashes when dragging tabs. Oct 8, 2024
@Oyyko
Copy link
Contributor

Oyyko commented Oct 8, 2024

@Minzihao 可以说一下为什么之前需要mutex现在不需要了吗?

@Minzihao
Copy link
Contributor Author

Minzihao commented Oct 9, 2024

@Minzihao 可以说一下为什么之前需要mutex现在不需要了吗?

之前使用mutex是防止拖动tab的过程中执行UI更新,因为UI更新会替换掉所有的tab窗口,导致持有一个无效tab指针,我以为这是导致crash的原因,然而,原因不仅如此。我在files changed中对代码评论了,你去看看

// drag->setPixmap (pixmap);
// drag->exec (Qt::MoveAction);
//
// setDown (false); // to avoid keeping the pressed state
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

112这行setDown (false);代码,原本是在110行drag->exec(Qt::MoveAction)之后执行的,然而在调试过程中我发现110行drag->exec(Qt::MoveAction)一旦执行后就有可能马上出发UI更新,导致tab窗口都被替换掉,这时候再执行112这行setDown (false);代码就会crash,因为this已经是无效指针


QPixmap pixmap (size ());
render (&pixmap);
setDown (false); // to avoid keeping the pressed state
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

因此我把setDown (false);这行代码提到上面

g_mostRecentlyDraggedTab= m_bufferUrl;

QDrag* drag=
new QDrag (parent ()); // don't point to `this`, it will cause crash
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

111行这里注意new QDrag (parent ());传递的参数是不是this,因为一旦发生UI更新,this就是无效指针,然而当用户在拖着tab但是还没放下时,我的理解时QDrag会访问this,这也会导致crash,所以我传递是parent ()即QTMTabContainer的指针

index= i;
break;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

为什么不需要mutex了?因为现在我将用户拖动的tab的url(文档的url)记录到g_mostRecentlyDraggedTab,这样即使在拖动的过程中发生了UI更新且所有tab窗口都被替换掉,也能追踪到拖动的那个tab,因为url不会变

}
}
if (index != -1) {
m_draggingTabIndex= index;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

然后记录这个拖动的tab在列表中的索引,在拖拽结束之前,这个索引一直都是有效的,因为在拖拽的过程中,不可能还能关掉某个tab吧,鼠标都还一直按着没有释放。所以,索引是有效的,不管发生多少次UI更新,都没有问题,这就不需要mutex了

Copy link
Contributor

@Oyyko Oyyko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Minzihao Minzihao merged commit bc4803e into branch-1.2 Oct 10, 2024
7 checks passed
@Minzihao Minzihao deleted the drag-and-sort-tabpage branch October 10, 2024 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants