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

Add feature to duplicate tabs #5277

Merged
merged 17 commits into from
May 25, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
41 changes: 40 additions & 1 deletion src/widgets/Notebook.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,12 @@ Notebook::Notebook(QWidget *parent)
}

NotebookTab *Notebook::addPage(QWidget *page, QString title, bool select)
{
return this->addPageAt(page, -1, std::move(title), select);
pajlada marked this conversation as resolved.
Show resolved Hide resolved
pajlada marked this conversation as resolved.
Show resolved Hide resolved
pajlada marked this conversation as resolved.
Show resolved Hide resolved
pajlada marked this conversation as resolved.
Show resolved Hide resolved
pajlada marked this conversation as resolved.
Show resolved Hide resolved
pajlada marked this conversation as resolved.
Show resolved Hide resolved
pajlada marked this conversation as resolved.
Show resolved Hide resolved
}

NotebookTab *Notebook::addPageAt(QWidget *page, int position, QString title,
pajlada marked this conversation as resolved.
Show resolved Hide resolved
bool select)
{
// Queue up save because: Tab added
getIApp()->getWindows()->queueSave();
Expand All @@ -101,7 +107,14 @@ NotebookTab *Notebook::addPage(QWidget *page, QString title, bool select)
item.page = page;
item.tab = tab;

this->items_.append(item);
if (position == -1)
{
this->items_.push_back(item);
}
else
{
this->items_.insert(position, item);
}

page->hide();
page->setParent(this);
Expand Down Expand Up @@ -165,6 +178,32 @@ void Notebook::removePage(QWidget *page)
this->performLayout(true);
}

void Notebook::duplicatePage(QWidget *page)
{
auto *item = this->findItem(page);
assert(item != nullptr);

auto *container = dynamic_cast<SplitContainer *>(item->page);
if (!container)
{
return;
}

auto descriptor = container->buildDescriptor();

auto *newContainer = new SplitContainer(this);
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(highlightState);

newContainer->setTab(tab);
}

void Notebook::removeCurrentPage()
{
if (this->selectedPage_ != nullptr)
Expand Down
9 changes: 9 additions & 0 deletions src/widgets/Notebook.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,16 @@ class Notebook : public BaseWidget

NotebookTab *addPage(QWidget *page, QString title = QString(),
bool select = false);

/**
* @brief Adds a page to the Notebook at a given position.
*
* @param position if set to -1, adds the page to the end
**/
NotebookTab *addPageAt(QWidget *page, int position = -1,
QString title = QString(), bool select = false);
void removePage(QWidget *page);
void duplicatePage(QWidget *page);
void removeCurrentPage();

/**
Expand Down
4 changes: 4 additions & 0 deletions src/widgets/helper/NotebookTab.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,10 @@ NotebookTab::NotebookTab(Notebook *notebook)
getIApp()->getHotkeys()->getDisplaySequence(HotkeyCategory::Window,
"popup", {{"window"}}));

this->menu_.addAction("Duplicate Tab", [this]() {
this->notebook_->duplicatePage(this->page);
});

highlightNewMessagesAction_ =
new QAction("Mark Tab as Unread on New Messages", &this->menu_);
highlightNewMessagesAction_->setCheckable(true);
Expand Down
55 changes: 54 additions & 1 deletion src/widgets/splits/SplitContainer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -762,6 +762,11 @@ SplitContainer::Node *SplitContainer::getBaseNode()
return &this->baseNode_;
}

NodeDescriptor SplitContainer::buildDescriptor()
pajlada marked this conversation as resolved.
Show resolved Hide resolved
{
return this->buildDescriptorRecursively(&this->baseNode_);
}

void SplitContainer::applyFromDescriptor(const NodeDescriptor &rootNode)
{
assert(this->baseNode_.type_ == Node::Type::EmptyRoot);
Expand Down Expand Up @@ -799,6 +804,54 @@ void SplitContainer::popup()
window.show();
}

NodeDescriptor SplitContainer::buildDescriptorRecursively(Node *currentNode)
pajlada marked this conversation as resolved.
Show resolved Hide resolved
pajlada marked this conversation as resolved.
Show resolved Hide resolved
pajlada marked this conversation as resolved.
Show resolved Hide resolved
pajlada marked this conversation as resolved.
Show resolved Hide resolved
{
if (currentNode->children_.empty())
{
const auto channelTypeToString = [](Channel::Type type) {
switch (type)
{
case Channel::Type::Twitch:
return "twitch";
case Channel::Type::TwitchWhispers:
return "whispers";
case Channel::Type::TwitchWatching:
return "watching";
case Channel::Type::TwitchMentions:
return "mentions";
case Channel::Type::TwitchLive:
return "live";
case Channel::Type::TwitchAutomod:
return "automod";
case Channel::Type::Irc:
return "irc";
case Channel::Type::Misc:
return "misc";
default:
return "";
}
};

SplitNodeDescriptor result;
result.type_ =
channelTypeToString(currentNode->split_->getChannel()->getType());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pajlada @Nerixyz not sure why but there is a Channel::Type inside IndirectChannel and inside Channel, and they are not matching here

image

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)

Copy link
Contributor

@Nerixyz Nerixyz May 18, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could get the changes from a patch and make a new commit out of that, is that ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

result.channelName_ = currentNode->split_->getChannel()->getName();
result.filters_ = currentNode->split_->getFilters();
return result;
}

ContainerNodeDescriptor descriptor;
for (auto &child : currentNode->children_)
{
descriptor.vertical_ =
currentNode->type_ == Node::Type::VerticalContainer;
descriptor.items_.push_back(
this->buildDescriptorRecursively(child.get()));
}

return descriptor;
}

void SplitContainer::applyFromDescriptorRecursively(
const NodeDescriptor &rootNode, Node *baseNode)
{
Expand Down Expand Up @@ -849,9 +902,9 @@ void SplitContainer::applyFromDescriptorRecursively(
}
const auto &splitNode = *inner;
auto *split = new Split(this);
split->setFilters(splitNode.filters_);
pajlada marked this conversation as resolved.
Show resolved Hide resolved
split->setChannel(WindowManager::decodeChannel(splitNode));
split->setModerationMode(splitNode.moderationMode_);
split->setFilters(splitNode.filters_);

auto *node = new Node();
node->parent_ = baseNode;
Expand Down
2 changes: 2 additions & 0 deletions src/widgets/splits/SplitContainer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ class SplitContainer final : public BaseWidget
void hideResizeHandles();
void resetMouseStatus();

NodeDescriptor buildDescriptor();
void applyFromDescriptor(const NodeDescriptor &rootNode);

void popup();
Expand All @@ -237,6 +238,7 @@ class SplitContainer final : public BaseWidget
void resizeEvent(QResizeEvent *event) override;

private:
NodeDescriptor buildDescriptorRecursively(Node *currentNode);
void applyFromDescriptorRecursively(const NodeDescriptor &rootNode,
Node *baseNode);

Expand Down