Closed gushromp closed 9 years ago
Hi! Great work!
I see many coding style issues. Please review Qt Creator Coding Rules.
It looks like commits e281999 and 6054e32 should be squashed. Also commit messages should be in present time ("Add a cool feature" instead of "Added a cool feature").
Are you comfortable with updating your pull request? If not, I'll do it myself later.
Thanks!
Thanks for the review!
I'll update this ASAP. Please bear with me as I'm not quite at home yet when it comes to using Git.
Sure, feel free to ask any questions. Please note, that I have updated the first comment.
Force push is the only way to update the pull request. What GitHub manual says, is that you should never forcibly push in to a permanent branch, like master
. I'll think about that index issue. In meanwhile, please squash commits and fix some coding style issues left. Thanks for your work!
Hi, I've updated everything as requested and hopefully squashed the excess commits.
Not sure why this one diff hasn't been marked as outdated even though it has been updated in one of the commits.
The idea of a pull request is that commits in it should not change each other's code. For example, fixing style changes should be merged with the commits that add the code needs fixing.
Anyway, I am still not sure about the actual implementation. Probably it would make more sense to use set contextMenuPolicy
to CustomContextMenu
or ActionsContextMenu
instead of reimplementing QTabWidget
. I shall play with your changes on weekends and see if there's a better or simpler way to achieve the same result.
If you manage to find a way to get the tab index via customContextMenuRequested
that'd be great but I couldn't find any way to do so myself. That signal only has a QPoint
parameter.
There's a way to avoid subclassing QTabWidget
by simply installing the same eventFilter
used in the custom class to an object of QTabWidget
class using installEventFilter(...)
.
Hello :)
I've removed usage of QSignalMapper
altogether and just went with action slots expecting no parameters and using m_currentTabIndex
instead.
I've snooped around core QtCreator code and it seems that indeed there is no other way but to store the selected tab index as a member variable. This same approach is used in FancyTabWidget
aka the QtCreator sidebar.
Also I've tried everything I could possibly come up with and searched the internet for an alternative to subclassing QTabWidget
for providing context menu but it actually seems impossible without subclassing (or directly installing an event filter for mouse events which seems even 'dirtier' to me).
Good. Then just squash your commits please.
All of them into a single commit?
The idea usually is, that the same code shouldn't be changed twice within one pull request. For example, coding style fixes should be definitely united with the corresponding commits.
I've decided, that the best option is to provide default context menus. That landed in 67d22508bef32cafe16b6d8e277f1d65891e1e92. That's way less code and more intuitive behaviour.
Agreed. Didn't even know a default one existed.
Hello.
I've added the context menu (right mouse button) on tabs and some common functionality such as: 1) Close all BUT this tab 2) Close all tabs to the right 3) Close all tabs
I've also fixed a bug where a tab would be closed even though the user has clicked Cancel on the Unsaved Edits dialog.