Some :tab-move related fixes #697

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
3 participants
@Carpetsmoker
Contributor

Carpetsmoker commented May 29, 2015

  • Only :tab-move + and :tab-move - worked; fix it so that :tab-move 1,
    :tab-move +2, or :tab-move -2 also works (as expected).
  • Don't give an error when moving before the first or beyond the last tab, just
    set it to start or last (IMHO this is much more sane, Vim also works like
    this).
  • :tab-move $ and $gm now work.

TODO:

  • Moving tabs resets the load status thingy next to the favicon? We need a new
    option for insertTab() in tabwidget.py I think
  • Make some tests
  • Code could be cleaner

This change is Reviewable

Martin Tournoij
Some :tab-move related fixes
- Only `:tab-move +` and `:tab-move -` worked; fix it so that `:tab-move 1`,
  `:tab-move +2`, or `:tab-move -2` also works (as expected).
- Don't give an error when moving before the first or beyond the last tab, just
  set it to start or last (IMHO this is much more sane, Vim also works like
  this).
- `:tab-move $` and `$gm` now work.

TODO:

- Moving tabs resets the load status thingy next to the favicon? We need a new
  option for `insertTab()` in `tabwidget.py` I think
- Make some tests
- Code could be cleaner
@The-Compiler

View changes

qutebrowser/browser/commands.py
@@ -851,7 +851,7 @@ def tab_focus(self, index: {'type': (int, 'last')}=None, count=None):
@cmdutils.register(instance='command-dispatcher', scope='window',
count='count')
- def tab_move(self, direction: {'type': ('+', '-')}=None, count=None):
+ def tab_move(self, direction: {'type': ('+', '-', str)}=None, count=None):

This comment has been minimized.

@The-Compiler

The-Compiler May 29, 2015

Collaborator

The type annotation here doesn't really do anything helpful anymore, as it takes any string now anyways - so let's just remove it.

@The-Compiler

The-Compiler May 29, 2015

Collaborator

The type annotation here doesn't really do anything helpful anymore, as it takes any string now anyways - so let's just remove it.

@The-Compiler

View changes

qutebrowser/browser/commands.py
@@ -860,7 +860,7 @@ def tab_move(self, direction: {'type': ('+', '-')}=None, count=None):
count: If moving absolutely: New position (default: 0)
If moving relatively: Offset.
"""
- if direction is None:
+ if direction == 'None':

This comment has been minimized.

@The-Compiler

The-Compiler May 29, 2015

Collaborator

After the annotation is gone, this should probably be is None again

@The-Compiler

The-Compiler May 29, 2015

Collaborator

After the annotation is gone, this should probably be is None again

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler May 29, 2015

Collaborator

I'm not sure what $ is supposed to do - does it just move the tab to the end?

Some other thoughts:

  • The docs for direction should probably be updated in the docstring.
  • As for tests, I started some tests in the command-tests branch, but then decided to work on tests for easier things first 😉
Collaborator

The-Compiler commented May 29, 2015

I'm not sure what $ is supposed to do - does it just move the tab to the end?

Some other thoughts:

  • The docs for direction should probably be updated in the docstring.
  • As for tests, I started some tests in the command-tests branch, but then decided to work on tests for easier things first 😉
@Carpetsmoker

This comment has been minimized.

Show comment
Hide comment
@Carpetsmoker

Carpetsmoker May 29, 2015

Contributor

I'm not sure what $ is supposed to do - does it just move the tab to the end?

Yeah. Same as in Vim :-)

Contributor

Carpetsmoker commented May 29, 2015

I'm not sure what $ is supposed to do - does it just move the tab to the end?

Yeah. Same as in Vim :-)

Some more fixes for :tab-move and related:
- $ is now a valid count; implement this also for :tab-focus
- Copy indicator colour when moving a tab
- Make :tab-close <n> work
@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jun 3, 2015

Collaborator

I took a quick look at the newest commit because I wondered what's new (but didn't look closer, so take this with a grain of salt):

Are you aware many commands can take a count? For some commands that selects a tab index already, e.g. pressing 1T will select the first tab.

I'm not sure if it really makes sense to duplicate this for commands where this is already possible using the count... Maybe it'd be better to add a generic --count argument to commands, so that can also be used via the commandline?

For some commands (like :tab-close), it seems like that diff would also break count support, which is definitely not desirable ;)

Collaborator

The-Compiler commented Jun 3, 2015

I took a quick look at the newest commit because I wondered what's new (but didn't look closer, so take this with a grain of salt):

Are you aware many commands can take a count? For some commands that selects a tab index already, e.g. pressing 1T will select the first tab.

I'm not sure if it really makes sense to duplicate this for commands where this is already possible using the count... Maybe it'd be better to add a generic --count argument to commands, so that can also be used via the commandline?

For some commands (like :tab-close), it seems like that diff would also break count support, which is definitely not desirable ;)

@Carpetsmoker

This comment has been minimized.

Show comment
Hide comment
@Carpetsmoker

Carpetsmoker Jun 3, 2015

Contributor

The "difficulty" is that count is only passed with the "normal mode" commands such as 2gm. But when you use :tab-move 2 it's not passed as count, but as direction, which is why :tab-move 2 or :tab-close 2 doesn't work at the moment...
For some commands (like :tab-focus) some games were already done with the argument order (ie. idx = cmdutils.arg_or_count(index, count)...

:tab-move --count 2 seems not only like more typing than is necessary, but also very surprising/counter-intuitive...

For some commands (like :tab-close), it seems like that diff would also break count support, which is definitely not desirable ;)

Hmm, it should work IIRC? But I didn't test very extensively as I'm not particularly happy with this solution as such (although I can't really think of anything significantly better at the moment, other than maybe adding a separate function for the normal mode command and ex command).

Contributor

Carpetsmoker commented Jun 3, 2015

The "difficulty" is that count is only passed with the "normal mode" commands such as 2gm. But when you use :tab-move 2 it's not passed as count, but as direction, which is why :tab-move 2 or :tab-close 2 doesn't work at the moment...
For some commands (like :tab-focus) some games were already done with the argument order (ie. idx = cmdutils.arg_or_count(index, count)...

:tab-move --count 2 seems not only like more typing than is necessary, but also very surprising/counter-intuitive...

For some commands (like :tab-close), it seems like that diff would also break count support, which is definitely not desirable ;)

Hmm, it should work IIRC? But I didn't test very extensively as I'm not particularly happy with this solution as such (although I can't really think of anything significantly better at the moment, other than maybe adding a separate function for the normal mode command and ex command).

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Oct 20, 2015

Collaborator

Reopening this as I still plan to take a closer look soon (tm)

Collaborator

The-Compiler commented Oct 20, 2015

Reopening this as I still plan to take a closer look soon (tm)

@The-Compiler The-Compiler reopened this Oct 20, 2015

@The-Compiler The-Compiler self-assigned this Oct 30, 2015

The-Compiler added a commit that referenced this pull request Jun 6, 2016

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jun 6, 2016

Collaborator

This took me longer than I'd like to admit, but I commited the tab indicator fix in 3e22f64.

As for the other things, I think they either are already implemented in a way or another, or are things I'm not sure I agree with, so I'm closing this again.

Collaborator

The-Compiler commented Jun 6, 2016

This took me longer than I'd like to admit, but I commited the tab indicator fix in 3e22f64.

As for the other things, I think they either are already implemented in a way or another, or are things I'm not sure I agree with, so I'm closing this again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment