Update oldschoolkeys, tabmenu, yanksel plugins #14

schvabodka-man · 2017-08-20T15:54:36Z

Hello there! Due to changes in API plugin tabmenu was broken and unusable on newer versions of luakit. I fixed it to work with new api and it works nice. I've also removed standart hotkey for it(because anyway you can set it in you luakit config and not everyone wants to have this hotkey). I also fixed yankselect and old school keybindings plugins. Would be sweat if you'll merge this

aidanholm

Mostly good, just a bunch of formatting changes really.
Make sure all tests pass when using these modules as well.

aidanholm · 2017-08-21T14:13:46Z

oldschoolkeys/init.lua

-}
+local add_binds = modes.add_binds
+
+module("plugins.oldschoolkeys")


The use of module() in plugins is now forbidden. This is because it unavoidably sets a global variable. Instead, declare a local table named _M and return it at the end of the file. All subsequent top-level variables should either be local or should be fields of _M.

aidanholm · 2017-08-21T14:24:33Z

oldschoolkeys/init.lua

@@ -4,24 +4,16 @@

 local pairs = pairs


Remove this

aidanholm · 2017-08-21T14:25:53Z

oldschoolkeys/init.lua

+
+module("plugins.oldschoolkeys")
+
+add_binds("normal",


Opening brace should go on the same line as add_binds, and closing brace+paren should go on a new line:

add_binds("normal", { ... })

aidanholm · 2017-08-21T14:27:08Z

tabmenu/init.lua

@@ -7,80 +7,71 @@ local ipairs = ipairs
 local table = table

 local lousy = require "lousy"
+


Remove space here

aidanholm · 2017-08-21T14:27:14Z

tabmenu/init.lua

@@ -7,80 +7,71 @@ local ipairs = ipairs
 local table = table

 local lousy = require "lousy"
+
+local modes = require "modes"
+


aidanholm · 2017-08-21T14:29:28Z

yanksel/init.lua

-local add_binds, add_cmds = add_binds, add_cmds
+
+local modes = require "modes"
+


aidanholm · 2017-08-21T14:29:41Z

yanksel/init.lua

+local modes = require "modes"
+
+local add_binds = modes.add_binds
+local add_cmds = modes.add_cmds

 module("plugins.yanksel")


ditto re module

aidanholm · 2017-08-21T14:31:12Z

yanksel/init.lua

-	    luakit.selection.primary = ""
-	end),
-})
+function yank_select(w)


I'd say make this an action table, which is just a table with func (function) and desc (description) keys. See lib/binds.lua for examples (scrolling actions).

aidanholm · 2017-08-21T14:31:33Z

tabmenu/init.lua

+local add_binds = modes.add_binds
+local add_cmds = modes.add_cmds
+local new_mode = modes.new_mode
+
 local add_binds, add_cmds = add_binds, add_cmds
 local new_mode, menu_binds = new_mode, menu_binds

 module("plugins.tabmenu")


ditto re module

aidanholm · 2017-08-21T14:31:49Z

tabmenu/init.lua

+local add_binds = modes.add_binds
+local add_cmds = modes.add_cmds
+local new_mode = modes.new_mode
+
 local add_binds, add_cmds = add_binds, add_cmds
 local new_mode, menu_binds = new_mode, menu_binds

 module("plugins.tabmenu")

 hide_box = false


make this _M.hide_box

schvabodka-man · 2017-08-21T15:30:09Z

Okay, here it goes. Check this one. Maybe in few time i'll fix user agent changer

aidanholm

Mostly fixed now

Separately, I'd prefer it if you split this fixes commit and squashed the changes for each file into the earlier file-specific commits, as this keeps the git history clean.

Finally, make sure the tests pass; to do this:

copy the modules into the luakit repo under config/
require these modules (require "oldschoolkeys", etc.) in config/rc.lua
run make run-tests

aidanholm · 2017-08-21T15:35:20Z

oldschoolkeys/init.lua

@@ -1,19 +1,17 @@
 ------------------------------------------------------------------------
 -- Add some convenient keybindings back.                              --
 ------------------------------------------------------------------------
-
-local pairs = pairs
 local lousy = require("lousy")



No blank lines between require lines

aidanholm · 2017-08-21T15:36:10Z

oldschoolkeys/init.lua

+add_binds("normal",{
+    { "b", "Back", function (w, m) w:back(m.count) end, {count=1}},
+    { "<Mod1-Page_Up>", "Reorder tabs", function (w, m) w.tabs:reorder(w.view, w.tabs:current() - m.count) end, {count=1}},
+    { "<Mod1-Page_Down>", "Reorder tabs", function (w, m) w.tabs:reorder(w.view, (w.tabs:current() + m.count) % w.tabs:count()) end, {count=1}}})


Final }) should go on a new line

aidanholm · 2017-08-21T15:36:39Z

oldschoolkeys/init.lua


+add_binds("normal",{
+    { "b", "Back", function (w, m) w:back(m.count) end, {count=1}},
+    { "<Mod1-Page_Up>", "Reorder tabs", function (w, m) w.tabs:reorder(w.view, w.tabs:current() - m.count) end, {count=1}},


These functions are long enough that they should be multi-line

aidanholm · 2017-08-21T15:38:22Z

yanksel/init.lua

-module("plugins.yanksel")
+local _M = {}
+
+local funcs = { 


This table should be called actions

schvabodka-man · 2017-08-21T15:57:16Z

It's passing tests

aidanholm · 2017-08-21T16:06:47Z

I'm getting the following test failures:

FAIL style/test_luacheck / test_luacheck
  tests/style/test_luacheck.lua:102: Luacheck messages:
    config/tabmenu/init.lua:28: (9:16):                setting non-standard global variable 'hide_box'
    config/tabmenu/init.lua:42: (12:19):               accessing undefined variable 'hide_box'
    config/oldschoolkeys/init.lua:4: (7:11):           unused variable 'lousy'
    config/yanksel/init.lua:5: (7:11):                 unused variable 'lousy'
    config/yanksel/init.lua:12: (18:18):               line contains trailing whitespace
FAIL style/test_source_format / test_vim_modeline
  tests/style/test_source_format.lua:41: Some files do not have modelines:
  config/tabmenu/init.lua                                Missing/malformed modeline
  config/oldschoolkeys/init.lua                          Missing/malformed modeline
  config/yanksel/init.lua                                Missing/malformed modeline
FAIL style/test_source_format / test_no_trailing_whitespace
  tests/style/test_source_format.lua:313: Some files have trailing whitespace:
  config/yanksel/init.lua:12                             Trailing whitespace
FAIL style/test_source_format / test_no_tabs_in_indentation
  tests/style/test_source_format.lua:287: Some files have tabs in indentation:
  config/tabmenu/init.lua:20                             Tabs in indentation
  config/oldschoolkeys/init.lua:14                       Tabs in indentation
  config/oldschoolkeys/init.lua:15                       Tabs in indentation
  config/oldschoolkeys/init.lua:16                       Tabs in indentation
  config/oldschoolkeys/init.lua:18                       Tabs in indentation
  config/oldschoolkeys/init.lua:19                       Tabs in indentation
  config/oldschoolkeys/init.lua:20                       Tabs in indentation
  config/yanksel/init.lua:14                             Tabs in indentation
  config/yanksel/init.lua:15                             Tabs in indentation
  config/yanksel/init.lua:16                             Tabs in indentation
  config/yanksel/init.lua:17                             Tabs in indentation
  config/yanksel/init.lua:18                             Tabs in indentation
  config/yanksel/init.lua:19                             Tabs in indentation
  config/yanksel/init.lua:20                             Tabs in indentation
  config/yanksel/init.lua:21                             Tabs in indentation

The trailing whitespace / tabs are easy to fix. Just remove the lines with unused variables.
For the modelines, you need to add the following to the bottom of each file, with a blank line above it:

-- vim: et:sw=4:ts=8:sts=4:tw=80

aidanholm · 2017-08-21T16:08:06Z

On tabmenu.lua lines 28 and 42, hide_box should be _M.hide_box; that should be everything!

schvabodka-man · 2017-08-21T16:19:01Z

I'm not using vim, i'm using emacs. I just replaced tabs with spaces(4 tabs = 4 spaces) and run it through lua beautifier. Hide box is already _M.hide_box(really it is). And 28 and 42 is intended with 4 spaces

schvabodka-man · 2017-08-21T16:21:09Z

Ok, wait for a second i fixed those references

aidanholm · 2017-08-21T16:23:28Z

Even so, the project's coding standard is to use vim modelines (and I thought emacs could read vim modelines?)
I'm guessing perhaps you forgot to push changes then? I have the same version as on this PR.

aidanholm · 2017-08-21T16:27:03Z

Ok, I see your fixes, let me try again

schvabodka-man · 2017-08-21T16:27:22Z

ok, i pushed, some more fixes. should be ok with this variables. And i know shit about these "cool" vim aliases. My emacs display these lines pretty nicely. Maybe in vim the're broken somehow
https://transfer.sh/14MtiM/screenshot.png

aidanholm · 2017-08-21T16:31:24Z

Ok, references are fixed, it's just whitespace errors and the extra require "lousy" lines now; once these are fixed it should be ready to merge!

aidanholm · 2017-08-21T16:33:06Z

Nice desktop btw; which wm?

schvabodka-man · 2017-08-21T16:35:28Z

I think it would be ok to merge all this fixes commits into three comits or not?

I3wm, polybar with bunch of scripts, rofi, Fedora(but most likely i would migrate to Void), fish shell and Emacs

Plaque-fcc · 2017-08-21T16:37:34Z

If they can be divided into three logically solid changesets — yes.

…

On August 21, 2017 7:35:29 PM GMT+03:00, Ivan ***@***.***> wrote: I think it would be ok to merge all this fixes commits into three comits or not? I3wm, polybar with bunch of scripts, rofi, Fedora(but most likely i would migrate to Void), fish shell and Emacs -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #14 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

schvabodka-man · 2017-08-21T16:40:00Z

I don't understand about which extra require "lousy" you're talking about. Projectile with grep gives me 4 require lousy(each for 1 plugin)

aidanholm · 2017-08-21T16:41:55Z

I'd also prefer that there be three commits, "Update tabmenu module" , "Update yanksel module", etc.
oldschoolkeys and yanksel never use the lousy variable, so it should be removed.

schvabodka-man · 2017-08-21T16:44:22Z

Fixed that one lousy

aidanholm · 2017-08-21T17:57:45Z

Just whitespace issues now; by the way once this is finished it'd be good to modify the README as well to say which plugins are updated and which aren't.

schvabodka-man · 2017-08-22T06:47:16Z

Can you point me on those whitespaces?

aidanholm · 2017-08-22T07:15:21Z

Have you ran the tests? That'll show you where the errors are, but they should be basically the same as in the test paste above. I'd just run sed -e 's/\t/ /g' -e 's/ $//' on oldschoolkeys/init.lua and yanksel/init.lua

Separately, the files are still missing modelines

schvabodka-man · 2017-08-22T16:12:03Z

I've run this sed command but it hasn't changed anything(diff is null).

schvabodka-man · 2017-08-23T06:41:06Z

Okay i've added modelines and i still don't know what whitespaces you're talking about

aidanholm · 2017-08-23T11:56:40Z

Sorry, I wasn't very clear when explaining before. There are two whitespace issues:

The lua files have tabs at the beginning of the line, as part of the indentation. This goes against the luakit code style standard, which is that all files must be indented with spaces only.

To find these files, run grep -rPn '\t' tabmenu oldschoolkeys yanksel from the luakit-plugins root folder. The easiest way to fix these problems is to do a find and replace in emacs, but you can also use the sed command I posted before.

Here is the output I get, that shows where the problems are. The file name and the line number are on the left, and the code with the whitespace error (tabs in the indentation) are on the right:

tabmenu/init.lua:20:	  { ":tabmenu", [[Open tab menu]], function (w) w:set_mode("tabmenu") end },
oldschoolkeys/init.lua:13:	  function (w, m)
oldschoolkeys/init.lua:14:		 w.tabs:reorder(w.view, w.tabs:current() - m.count)
oldschoolkeys/init.lua:15:	  end, {count=1}},
oldschoolkeys/init.lua:17:	  function (w, m)
oldschoolkeys/init.lua:18:		 w.tabs:reorder(w.view, (w.tabs:current() + m.count) % w.tabs:count())
oldschoolkeys/init.lua:19:	  end, {count=1}}
yanksel/init.lua:13:	  desc = "Yank selection.",
yanksel/init.lua:14:	  func = function (w)
yanksel/init.lua:15:		 local text = luakit.selection.primary
yanksel/init.lua:16:		 if not text then w:error("Empty selection.") return end
yanksel/init.lua:17:		 luakit.selection.clipboard = text
yanksel/init.lua:18:		 w:notify("Yanked selection: " .. text)
yanksel/init.lua:19:		 luakit.selection.primary = ""
yanksel/init.lua:20:	  end,

Trailing whitespace; there's a space at the end of a line in the yanksel/init.lua file. This is also not allowed by the current code standards, as it makes diffs larger than necessary, which makes it more difficult to read them and can cause issues with git blame.

The problem is at line 11 of yanksel/init.lua (local actions = {). There's only one of these, so it's simple to fix manually, with the backspace key :)

Let me know if that's not clear!

Plaque-fcc · 2017-08-23T11:59:32Z

Guess it was my bad on tab indents. :<

…

On August 23, 2017 2:56:40 PM GMT+03:00, Aidan Holm ***@***.***> wrote: Sorry, I wasn't very clear when explaining before. There are two whitespace issues: - The lua files have tabs at the beginning of the line, as part of the indentation. This goes against the luakit code style standard, which is that all files must be indented with spaces only. To find these files, run `grep -rPn '\t' tabmenu oldschoolkeys yanksel` from the `luakit-plugins` root folder. The easiest way to fix these problems is to do a find and replace in emacs, but you can also use the sed command I posted before. Here is the output I get, that shows where the problems are: ``` tabmenu/init.lua:20: { ":tabmenu", [[Open tab menu]], function (w) w:set_mode("tabmenu") end }, oldschoolkeys/init.lua:13: function (w, m) oldschoolkeys/init.lua:14: w.tabs:reorder(w.view, w.tabs:current() - m.count) oldschoolkeys/init.lua:15: end, {count=1}}, oldschoolkeys/init.lua:17: function (w, m) oldschoolkeys/init.lua:18: w.tabs:reorder(w.view, (w.tabs:current() + m.count) % w.tabs:count()) oldschoolkeys/init.lua:19: end, {count=1}} yanksel/init.lua:13: desc = "Yank selection.", yanksel/init.lua:14: func = function (w) yanksel/init.lua:15: local text = luakit.selection.primary yanksel/init.lua:16: if not text then w:error("Empty selection.") return end yanksel/init.lua:17: luakit.selection.clipboard = text yanksel/init.lua:18: w:notify("Yanked selection: " .. text) yanksel/init.lua:19: luakit.selection.primary = "" yanksel/init.lua:20: end, ``` - Trailing whitespace; there's a space at the end of a line in the `yanksel/init.lua` file. This is also not allowed by the current code standards, as it makes diffs larger than necessary, which makes it more difficult to read them and can cause issues with `git blame`. The problem is at line 11 of `yanksel/init.lua` (`local actions = {`). Let me know if that's not clear! -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #14 (comment)

-- J.K.

aidanholm · 2017-08-23T12:00:47Z

@Plaque-fcc no worries, there weren't any tests for that kind of thing with the old luakit, and most things had a mix of tabs and spaces :)

Plaque-fcc · 2017-08-23T12:03:51Z

No, it was my first meet of Lua-driven software. And I still thank @mason-larobina for this.

…

On August 23, 2017 3:00:48 PM GMT+03:00, Aidan Holm ***@***.***> wrote: @Plaque-fcc no worries, there weren't any tests for that kind of thing with the old luakit, and most things had a mix of tabs and spaces :) -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #14 (comment)

-- J.K.

aidanholm · 2017-08-26T15:50:57Z

Hey @schvabodka-man, any progress on this one? I'm happy to help if you have any questions

schvabodka-man · 2017-08-27T04:55:24Z

No, progress. I'm busy now

aidanholm · 2017-08-27T08:32:44Z

No problem

schvabodka-man · 2017-08-27T09:44:45Z

Maybe in 2-3 days i will actually start to work on these whitespaces

aidanholm · 2017-09-01T17:45:02Z

I've merged this one manually; thanks @schvabodka-man for updating these :)

schvabodka-man · 2017-09-02T09:52:52Z

Thx, i really appreciate your cleanup on whitespaces cause i don't have much time now

aidanholm · 2017-09-02T14:03:20Z

No problem

schvabodka-man added 3 commits August 20, 2017 18:51

fixed tabmenu

b34109c

fixed yankselect

ca487c8

fixed old school keybindings

0baa3d6

schvabodka-man mentioned this pull request Aug 21, 2017

Plugins in luakit-plugins repo are broken luakit/luakit#457

Open

aidanholm requested changes Aug 21, 2017

View reviewed changes

fixes

4893fb0

aidanholm requested changes Aug 21, 2017

View reviewed changes

fixes

9dd98a4

fixes

24395e9

fixes

4e7c5c3

fixes

52c2752

vim modelines

a918b94

aidanholm mentioned this pull request Aug 23, 2017

Update useragent strings in uaswitch.lua #16

Open

aidanholm changed the title ~~Fixed broken tabmenu plugin~~ Update oldschoolkeys, tabmenu, yanksel plugins Aug 23, 2017

aidanholm mentioned this pull request Aug 23, 2017

tabmenu plugin doesn't work now #13

Closed

Merge branch 'master' into master

7996896

aidanholm closed this Sep 1, 2017

		@@ -7,80 +7,71 @@ local ipairs = ipairs
		local table = table

		local lousy = require "lousy"

		local add_binds, add_cmds = add_binds, add_cmds

		local modes = require "modes"


		module("plugins.oldschoolkeys")

		add_binds("normal",

Update oldschoolkeys, tabmenu, yanksel plugins #14

Update oldschoolkeys, tabmenu, yanksel plugins #14

Conversation

schvabodka-man commented Aug 20, 2017 • edited Loading

aidanholm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schvabodka-man commented Aug 21, 2017

aidanholm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schvabodka-man commented Aug 21, 2017

aidanholm commented Aug 21, 2017

aidanholm commented Aug 21, 2017

schvabodka-man commented Aug 21, 2017

schvabodka-man commented Aug 21, 2017

aidanholm commented Aug 21, 2017

aidanholm commented Aug 21, 2017

schvabodka-man commented Aug 21, 2017

aidanholm commented Aug 21, 2017

aidanholm commented Aug 21, 2017

schvabodka-man commented Aug 21, 2017

Plaque-fcc commented Aug 21, 2017 via email

schvabodka-man commented Aug 21, 2017

aidanholm commented Aug 21, 2017

schvabodka-man commented Aug 21, 2017

aidanholm commented Aug 21, 2017

schvabodka-man commented Aug 22, 2017

aidanholm commented Aug 22, 2017

schvabodka-man commented Aug 22, 2017

schvabodka-man commented Aug 23, 2017

aidanholm commented Aug 23, 2017 • edited Loading

Plaque-fcc commented Aug 23, 2017 via email

aidanholm commented Aug 23, 2017

Plaque-fcc commented Aug 23, 2017 via email

aidanholm commented Aug 26, 2017 • edited Loading

schvabodka-man commented Aug 27, 2017

aidanholm commented Aug 27, 2017

schvabodka-man commented Aug 27, 2017

aidanholm commented Sep 1, 2017

schvabodka-man commented Sep 2, 2017

aidanholm commented Sep 2, 2017

schvabodka-man commented Aug 20, 2017 •

edited

Loading

aidanholm commented Aug 23, 2017 •

edited

Loading

aidanholm commented Aug 26, 2017 •

edited

Loading