Add feature to duplicate tabs #5277
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
KleberPF
force-pushed
the
feature-duplicate-tab
branch
from
879dba1 to
912fcdc
Compare
KleberPF
commented
Mar 30, 2024
KleberPF
force-pushed
the
feature-duplicate-tab
branch
from
1c14542 to
aa78745
Compare
|
This currently crashes if you try to duplicate a previously duplicated tab - something with attempting to access the clicked item's highlight state - gdb: valgrind: |
Nerixyz
reviewed
May 18, 2024
src/widgets/Notebook.cpp
Outdated
| auto *tab = this->addPageAt( | ||
| newContainer, newTabPosition, | ||
| item->tab->hasCustomTitle() ? item->tab->getCustomTitle() : "", false); | ||
| tab->setHighlightState(item->tab->highlightState()); |
There was a problem hiding this comment.
This currently crashes if you try to duplicate a previously duplicated tab - something with attempting to access the clicked item's highlight state -
item->tabseems to be a nullptr/invalid ptr
item is invalid.
This accesses item after it's invalidated due to a resize of items_ by addPageAt (item points to a location inside that list). The following fixes the crash:
diff --git a/src/widgets/Notebook.cpp b/src/widgets/Notebook.cpp
index 0350a1d7..4bbc6148 100644
--- a/src/widgets/Notebook.cpp
+++ b/src/widgets/Notebook.cpp
@@ -195,10 +195,11 @@ void Notebook::duplicatePage(QWidget *page)
newContainer->applyFromDescriptor(descriptor);
int newTabPosition = this->indexOf(page) + 1;
+ auto highlightState = item->tab->highlightState();
auto *tab = this->addPageAt(
newContainer, newTabPosition,
item->tab->hasCustomTitle() ? item->tab->getCustomTitle() : "", false);
- tab->setHighlightState(item->tab->highlightState());
+ tab->setHighlightState(highlightState);
newContainer->setTab(tab);
}There was a problem hiding this comment.
I knew I would be missing something, thanks.
|
issue while duplicate a tab with a |
KleberPF
force-pushed
the
feature-duplicate-tab
branch
from
0184bc4 to
e786dc1
Compare
I'll look into it, thanks. |
KleberPF
commented
May 18, 2024
|
|
||
| SplitNodeDescriptor result; | ||
| result.type_ = | ||
| channelTypeToString(currentNode->split_->getChannel()->getType()); |
There was a problem hiding this comment.
@pajlada @Nerixyz not sure why but there is a Channel::Type inside IndirectChannel and inside Channel, and they are not matching here
The reason being that when the watchingChannel is created, it gets passed an empty channel that has type None
watchingChannel(Channel::getEmpty(), Channel::Type::TwitchWatching)
There was a problem hiding this comment.
The type of the IndirectChannel is the type of indirection. It's really only for the watching channel. Its type is only ever TwitchWatching or Direct (don't quote me on that). Maybe there should be another type for the type of indirection, but it's easier for saving/restoring right now, I suppose.
If you use getIndirectChannel().getType() instead if getChannel()->getType() it seems to work.
kornes
reviewed
May 18, 2024
|
|
||
| SplitNodeDescriptor result; | ||
| result.type_ = | ||
| channelTypeToString(currentNode->split_->getChannel()->getType()); |
There was a problem hiding this comment.
descriptor building doesn't handle empty nodes rn, so trying to duplicate empty tab (Node::Type::EmptyRoot) will crash here because split is null. We probably should allow that for ux consistency
There was a problem hiding this comment.
Looks like you force pushed Pajs commits out of existence, can look for them upward from here 44f22f7 :D
Oops, my alias did a bit of trolling. Didn't realize someone else pushed to the branch. Not sure if there is an easy way for me to recover it now that the commit is orphan
descriptor building doesn't handle empty nodes rn, so trying to duplicate empty tab (Node::Type::EmptyRoot) will crash here because split is null. We probably should allow that for ux consistency
Makes sense, I'll add a check for that
There was a problem hiding this comment.
Oops, my alias did a bit of trolling. Didn't realize someone else pushed to the branch. Not sure if there is an easy way for me to recover it now that the commit is orphan
If you know the SHA, you can append .diff or .patch to the commit's URL on GitHub - for example https://github.com/Chatterino/chatterino2/commit/44f22f732559ab3065d5fcadb33028b3ad5faec8.patch.
There was a problem hiding this comment.
I could get the changes from a patch and make a new commit out of that, is that ok?
There was a problem hiding this comment.
If you know the SHA, you can append .diff or .patch to the commit's URL on GitHub
We commented basically at the same time lol. Yeah, I will do that and add a new commit with pajlada's changes.
There was a problem hiding this comment.
patches are fine, rebasing onto 44f22f7 should also work or cherry picking skipped commits
git cherry-pick 2f4021bfd9c1b5e156942e3bc8b880524fb5e96c
git cherry-pick 75027efb07517b70a1a535de97e1f9c6408b17e3
git cherry-pick bfce89c94d9858b2bf4fc53c63971491d4a842e7
git cherry-pick 470cd2ae33114cbb7e83a9996653500e42b484c2
git cherry-pick 0184bc49446f2f6b5f159c268c7e7973eb2b45f0
There was a problem hiding this comment.
I wasn't able to find the commits by fetching (I guess because they aren't on any branch) so cherry-pick wasn't working
$ git cherry-pick 470cd2ae33114cbb7e83a9996653500e42b484c2
fatal: bad object 470cd2ae33114cbb7e83a9996653500e42b484c2
|
@8thony it should work now, could you test it again? |
8thony
approved these changes
May 18, 2024
pajlada
reviewed
May 19, 2024
There was a problem hiding this comment.
segfault when trying to dupe an empty tab
Thread 1 "chatterino" received signal SIGSEGV, Segmentation fault.
0x00005555562ee228 in std::__shared_ptr<chatterino::IndirectChannel::Data, (__gnu_cxx::_Lock_policy)2>::__shared_ptr (
this=0x7fffffff3bf0)
at /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/14.1.1/../../../../include/c++/14.1.1/bits/shared_ptr_base.h:1523
1523 __shared_ptr(const __shared_ptr&) noexcept = default;
(gdb) bt
#0 0x00005555562ee228 in std::__shared_ptr<chatterino::IndirectChannel::Data, (__gnu_cxx::_Lock_policy)2>::__shared_ptr
(this=0x7fffffff3bf0)
at /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/14.1.1/../../../../include/c++/14.1.1/bits/shared_ptr_base.h:1523
#1 0x00005555562ee1fd in std::shared_ptr<chatterino::IndirectChannel::Data>::shared_ptr (this=0x7fffffff3bf0)
at /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/14.1.1/../../../../include/c++/14.1.1/bits/shared_ptr.h:203
#2 0x00005555562e845d in chatterino::IndirectChannel::IndirectChannel (this=0x7fffffff3bf0)
at /home/pajlada/git/chatterino2/src/common/Channel.hpp:131
#3 0x000055555652aa97 in chatterino::Split::getIndirectChannel (this=0x0)
at /home/pajlada/git/chatterino2/src/widgets/splits/Split.cpp:858
#4 0x0000555556557a5e in chatterino::SplitContainer::buildDescriptorRecursively
(this=0x555558bcb9f0, currentNode=0x555558bcbb90) at /home/pajlada/git/chatterino2/src/widgets/splits/SplitContainer.cpp:820
#5 0x000055555655798a in chatterino::SplitContainer::buildDescriptor (this=0x555558bcb9f0)
at /home/pajlada/git/chatterino2/src/widgets/splits/SplitContainer.cpp:769
#6 0x000055555634cffe in chatterino::Notebook::duplicatePage (this=0x55555698d7b0, page=0x555558bcb9f0)
at /home/pajlada/git/chatterino2/src/widgets/Notebook.cpp:192
#7 0x000055555645bc96 in chatterino::NotebookTab::NotebookTab(chatterino::Notebook*)::$_4::operator()() const
(this=0x55555818bdd0) at /home/pajlada/git/chatterino2/src/widgets/helper/NotebookTab.cpp:102
#8 0x000055555645bc36 in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, chatterino::NotebookTab::NotebookTab(chatterino::Notebook*)::$_4>::call(chatterino::NotebookTab::NotebookTab(chatterino::Notebook*)::$_4&, void**)
(f=..., arg=0x7fffffff3ff0) at /usr/include/qt6/QtCore/qobjectdefs_impl.h:137
#9 0x000055555645bbf1 in QtPrivate::FunctorCallable<chatterino::NotebookTab::NotebookTab(chatterino::Notebook*)::$_4>::call<QtPrivate::List<>, void>(chatterino::NotebookTab::NotebookTab(chatterino::Notebook*)::$_4&, void*, void**) (f=..., arg=0x7fffffff3ff0)
at /usr/include/qt6/QtCore/qobjectdefs_impl.h:345
#10 0x000055555645bb8e in QtPrivate::QCallableObject<chatterino::NotebookTab::NotebookTab(chatterino::Notebook*)::$_4, QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*)
(which=1, this_=0x55555818bdc0, r=0x555558112a10, a=0x7fffffff3ff0, ret=0x0)
at /usr/include/qt6/QtCore/qobjectdefs_impl.h:555
#11 0x00007ffff679b57f in ??? () at /usr/lib/libQt6Core.so.6
#12 0x00007ffff7301eda in QAction::activate(QAction::ActionEvent) () at /usr/lib/libQt6Gui.so.6
#13 0x00007ffff7ac5029 in ??? () at /usr/lib/libQt6Widgets.so.6
#14 0x00007ffff7ac6942 in ??? () at /usr/lib/libQt6Widgets.so.6
#15 0x00007ffff7949a99 in QWidget::event(QEvent*) () at /usr/lib/libQt6Widgets.so.6
#16 0x00007ffff78fc44d in QApplicationPrivate::notify_helper(QObject*, QEvent*) () at /usr/lib/libQt6Widgets.so.6
#17 0x00007ffff7901548 in QApplication::notify(QObject*, QEvent*) () at /usr/lib/libQt6Widgets.so.6
#18 0x00007ffff673fe18 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () at /usr/lib/libQt6Core.so.6
#19 0x00007ffff78f457b in QApplicationPrivate::sendMouseEvent(QWidget*, QMouseEvent*, QWidget*, QWidget*, QWidget**, QPointer<QWidget>&, bool, bool) () at /usr/lib/libQt6Widgets.so.6
#20 0x00007ffff795f484 in ??? () at /usr/lib/libQt6Widgets.so.6
#21 0x00007ffff7960370 in ??? () at /usr/lib/libQt6Widgets.so.6
#22 0x00007ffff78fc44d in QApplicationPrivate::notify_helper(QObject*, QEvent*) () at /usr/lib/libQt6Widgets.so.6
#23 0x00007ffff673fe18 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () at /usr/lib/libQt6Core.so.6
#24 0x00007ffff6f75b20 in QGuiApplicationPrivate::processMouseEvent(QWindowSystemInterfacePrivate::MouseEvent*) ()
at /usr/lib/libQt6Gui.so.6
#25 0x00007ffff6fe8dcc in QWindowSystemInterface::sendWindowSystemEvents(QFlags<QEventLoop::ProcessEventsFlag>) ()
at /usr/lib/libQt6Gui.so.6
#26 0x00007ffff23e6cb7 in ??? () at /usr/lib/qt6/plugins/platforms/../../../libQt6XcbQpa.so.6
#27 0x00007ffff57e4a89 in ??? () at /usr/lib/libglib-2.0.so.0
#28 0x00007ffff58469b7 in ??? () at /usr/lib/libglib-2.0.so.0
#29 0x00007ffff57e3f95 in g_main_context_iteration () at /usr/lib/libglib-2.0.so.0
#30 0x00007ffff6993389 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) ()
at /usr/lib/libQt6Core.so.6
#31 0x00007ffff6748350 in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/libQt6Core.so.6
#32 0x00007ffff6743c1d in QCoreApplication::exec() () at /usr/lib/libQt6Core.so.6
#33 0x0000555555b8e023 in chatterino::Application::run (this=0x7fffffff5310, qtApp=...)
at /home/pajlada/git/chatterino2/src/Application.cpp:326
#34 0x0000555555bfefc2 in chatterino::runGui (a=..., paths=..., settings=..., args=..., updates=...)
at /home/pajlada/git/chatterino2/src/RunGui.cpp:281
#35 0x0000555555b6d23b in main (argc=1, argv=0x7fffffffe358) at /home/pajlada/git/chatterino2/src/main.cpp:102
currentNode->split_ is a nullptr
I tested duplicating an empty tab, but not duplicating a duplicated empty tab. I added a check now to only try to build and apply from the descriptor if we have a split in the tab. |
this shouldn't be able to fail, but this is a bit more "defensive"
pajlada
approved these changes
May 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements tab duplication by creating a descriptor of the tab and applying that descriptor to a new
SplitContainer.The rightmost tab here has a filter to only show messages sent by me.
Partially implements #5221