akhodakivskiy / VimFx

Vim keyboard shortcuts for Firefox
https://addons.mozilla.org/firefox/addon/vimfx
Other
1.42k stars 174 forks source link

fix: use new pinned tab property #1008

Open alexnguyennn opened 1 week ago

alexnguyennn commented 1 week ago

noticed g^ that accounted for number of pinned tabs stopped working with latest VimFx in Firefox Developer Edition 133.0b6.

After console.log printing the gBrowser object I found it seems to list the previous _numPinnedTabs property as undefined and pinnedTabCount now filled in so did a codebase-wide s/_numPinnedTabs/pinnedTabCount.

Seems to work for me in custom functions, would appreciate another opinion to test/confirm worthy of upstreaming.

Thanks!

girst commented 6 days ago

thanks for the patch!

vimfx currently targets firefox 115 and above. as to not increase the minimum version, please make the changes backwards compatible, and document them, like so:

numPinnedTabs = gBrowser.pinnedTabCount or gBrowser._numPinnedTabs # fx

where N is the last number that requires _numPinnedTabs (guessing, 132?).

you can grep for 'fx126' or 'fx127' to find other instances where such code was necessary, in case you need inspiration :)

On Tue, Nov 19, 2024 at 02:47:47PM -0800, alexnguyennn wrote:

noticed g^ that accounted for number of pinned tabs stopped working with latest VimFx in Firefox Developer Edition 133.0b6.

After console.log printing the gBrowser object I found it seems to list the previous _numPinnedTabs property as undefined and pinnedTabCount now filled in so did a codebase-wide s/_numPinnedTabs/pinnedTabCount.

Seems to work for me in custom functions, would appreciate another opinion to test/confirm worthy of upstreaming.

Thanks! You can view, comment on, or merge this pull request online at:

https://github.com/akhodakivskiy/VimFx/pull/1008

-- Commit Summary --

  • fix: use new pinned tab property

-- File Changes --

M extension/lib/commands.coffee (4)

-- Patch Links --

https://github.com/akhodakivskiy/VimFx/pull/1008.patch https://github.com/akhodakivskiy/VimFx/pull/1008.diff

-- Reply to this email directly or view it on GitHub: https://github.com/akhodakivskiy/VimFx/pull/1008 You are receiving this because you are subscribed to this thread.

Message ID: @.***>

girst commented 6 days ago

in case you're wondering (and you might, given that I have never written this down), the informal policy i've adopted is:

alexnguyennn commented 2 days ago

makes sense, thanks for the feedback - updated per your suggestion

girst commented 2 days ago

there's a linting error, identifed by npm run gulp lint.

lib/commands.coffee
  ✖  line 399  Line exceeds maximum allowed length  Length is 99, max is 80

✖ 1 error

[12:31:26] 'lint-only' errored after 1.77 s

i would definitely appreciate it if you'd address it, but if you don't want to, i'll fix it before the next release (which will come after bug 1007 is fixed).

girst commented 2 days ago

On Sun, Nov 24, 2024 at 05:31:21AM -0800, Simon Lydell wrote:

You probably want ? rather than or so that numPinnedTabs does not end up being undefined when gBrowser.pinnedTabCount is 0.

thanks for your input (and nice to see you still around :) ). definitely did not know that. the existing uses of this pattern seem safe, as they are applied to functions, not numbers.