-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feature: add close tabs to the right, #822 #875
Conversation
Thank you for handing in this pull request! The failed CI build is probably not your fault. I'm looking into that right now. |
Did you get a chance to look into it? |
I'm looking into it now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation looks fine. Please fix the formatting and white spacing issues. Thanks for implementing!
#include <QtContextMenu.h> | ||
#include <QtGraphicsView.h> | ||
#include <logging.h> | ||
#include <QContextMenuEvent> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow convention:
For Qt headers we don't add '.h' e.g. #include <QContextMenuEvent>
For own headers we use double quotations e.g. #include "QtContextMenu.h"
Qt headers are placed above own headers (see other files)
@@ -76,6 +76,7 @@ private slots: | |||
|
|||
void legendClicked(); | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this change.
src/lib_gui/qt/element/QtTabBar.cpp
Outdated
|
||
void QtTabBar::contextMenuEvent(QContextMenuEvent* event) | ||
{ | ||
QtContextMenu menu(event, this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your whole implementation uses the wrong indentation. Please change everything to tabs
instead of spaces
.
thanks for the feedback. will make the changes and then turn on to the next issue... |
Yes, I managed to fix the CI. So once you made the changes @egraether requested, you can make a new commit and the CI will automatically re-run the checks on your pull request. |
So, i hope this is it :) |
Thanks! |
Please let me know if this is ok. If something is missing just tell me.
As far as i can see this works just fine.
I have no automated test for this. Is this required?