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

feat: remove the limit of term splitting #353

Merged
merged 9 commits into from
Sep 8, 2024

Conversation

hualet
Copy link
Contributor

@hualet hualet commented Aug 22, 2024

a feature request from here: https://bbs.deepin.org/post/277237

Copy link

github-actions bot commented Aug 22, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@hualet
Copy link
Contributor Author

hualet commented Aug 22, 2024

I have read the CLA Document and I hereby sign the CLA.

@rb-union
Copy link

/check clacheck/CLAssistant

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Aug 23, 2024

Deepin Obs Bot
Starting find obs webhook event and retrigger!

@rb-union
Copy link

rb-union commented Aug 23, 2024

setMinimumSize(MainWindow::m_MinWidth / 2, (MainWindow::m_MinHeight - WIN_TITLE_BAR_HEIGHT) / 2);

这里有设置最小的大小限制,小窗口下显示分屏多的话,下面的分屏终端显示会被遮盖。

@BLumia
Copy link
Member

BLumia commented Aug 23, 2024

recheck

@hualet
Copy link
Contributor Author

hualet commented Aug 23, 2024

I have read the CLA Document and I hereby sign the CLA.

@hualet hualet force-pushed the remove_split_count_limit branch from 23bf46b to 5516f54 Compare August 26, 2024 05:47
@hualet
Copy link
Contributor Author

hualet commented Aug 27, 2024

  • 在中间某个term上分屏,新终端出现的位置不对
  • 拖动分割线以后,终端分屏的尺寸不对
  • 在无法分屏的时候(如何判断?),隐藏或者禁用分屏菜单项

hualet added 4 commits August 28, 2024 18:23
fix no term gets focus after closing one, which will cause crashes
if you hit Alt+Q in sequence.
original minimum size relies on the limit of one can only split
the terminal once, since we removed the limits, the minimum size
should change accordingly
fix the bug that new term not showing right after the old one after
the splitting
@hualet hualet force-pushed the remove_split_count_limit branch from 5516f54 to 6d03e2c Compare August 29, 2024 16:07
fix the bug that new term has wrong size after splitting
@hualet hualet force-pushed the remove_split_count_limit branch from 6d03e2c to eee66bc Compare August 29, 2024 16:10
@hualet
Copy link
Contributor Author

hualet commented Aug 30, 2024

@rb-union @ArchieMeng done!

Copy link
Contributor

@ArchieMeng ArchieMeng left a comment

Choose a reason for hiding this comment

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

Basically LGTM.
Additionally, a commit message is required by https://github.com/linuxdeepin/deepin-terminal/actions/runs/10635689078/job/29485856049?pr=353

src/views/termwidget.h Outdated Show resolved Hide resolved
src/views/termwidget.h Outdated Show resolved Hide resolved
disable splitting menu when there's no enough room for splitting
fix shortcut still work even when there's no room for splitting
createSubSplit has side effects, it removes the term form its
parent, so find size of it in QSplitter::sizes crashes.
@hualet hualet force-pushed the remove_split_count_limit branch from 12de9a0 to f7d6de1 Compare September 2, 2024 15:30
@deepin-ci-robot
Copy link

deepin pr auto review

关键摘要:

  • TermWidget::canSplit函数中,使用了qDebug宏,建议使用更合适的日志记录方法。
  • TermWidgetPage::setSplitStyle函数中,对QSplitterHandle的设置重复,可以考虑重构以减少重复代码。
  • TermWidgetPage::split函数中,处理splitternullptr的情况时,没有检查term是否为nullptr,可能会导致空指针解引用。
  • TermWidgetPage::closeSplit函数中,对upSplit的类型转换为QSplitter后没有检查转换结果,可能会导致运行时错误。
  • TermWidgetPage::closeSplit函数中,对brother的类型转换为TermWidget后没有检查转换结果,可能会导致运行时错误。
  • TermWidgetPage::closeSplit函数中,对nextTerm的类型转换为TermWidget后没有检查转换结果,可能会导致运行时错误。
  • TermWidgetPage::setCurrentTerminal函数中,直接调用setFocus可能不会按预期工作,因为setFocus可能需要在合适的上下文中调用。
  • TermWidgetPage::setCurrentTerminal函数中,没有处理termnullptr的情况,可能会导致未定义行为。

是否建议立即修改:

@@ -1131,7 +1131,7 @@ inline void MainWindow::slotShortcutHorizonzalSplit()
// 判读数量是否允许分屏
if (Service::instance()->isCountEnable()) {
TermWidgetPage *page = currentPage();
if (page && CanSplit(page->currentTerminal(), Qt::Vertical)) {
if (page && TermWidget::canSplit(page->currentTerminal(), Qt::Vertical)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I meant "convert canSplit to object function". So that, the way of calling it becomes

page->currentTerminal()->canSplit(Qt::Vertical)

@@ -1011,6 +1011,38 @@ bool TermWidget::isInRemoteServer()
return false;
}

bool TermWidget::canSplit(TermWidget *term, Qt::Orientation ori) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I meant "convert canSplit to object function".

- change CanSplit to a public funtion of TermWidget
- change MIN_WIDTH and MIN_HEIGHT of TermWidget to private
@ArchieMeng ArchieMeng force-pushed the remove_split_count_limit branch from f7d6de1 to 084e086 Compare September 8, 2024 01:29
Copy link
Contributor

@ArchieMeng ArchieMeng left a comment

Choose a reason for hiding this comment

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

+1

@ArchieMeng ArchieMeng merged commit 0a71657 into linuxdeepin:master Sep 8, 2024
16 checks passed
@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ArchieMeng, hualet

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mhduiy pushed a commit to mhduiy/deepin-terminal that referenced this pull request Oct 22, 2024
* feat: remove the limit of term splitting

a feature request from here: https://bbs.deepin.org/post/277237

* fix: no term gets focus after closing one

fix no term gets focus after closing one, which will cause crashes
if you hit Alt+Q in sequence.

* fix: change minimum size set on TermWidget

original minimum size relies on the limit of one can only split
the terminal once, since we removed the limits, the minimum size
should change accordingly

* fix: new term not showing in the right position

fix the bug that new term not showing right after the old one after
the splitting

* fix: new term resized wrongly

fix the bug that new term has wrong size after splitting

* fix: disable splitting menu if there's no room

disable splitting menu when there's no enough room for splitting

* fix: shortcut can still do splitting on room left

fix shortcut still work even when there's no room for splitting

* fix: crash of unfound size

createSubSplit has side effects, it removes the term form its
parent, so find size of it in QSplitter::sizes crashes.

* chore: some code refactory

- change CanSplit to a public funtion of TermWidget
- change MIN_WIDTH and MIN_HEIGHT of TermWidget to private
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.

5 participants