New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RDY] Keep unnamed register on restart #4700

Merged
merged 9 commits into from May 31, 2017

Conversation

Projects
None yet
5 participants
@AdnoC
Contributor

AdnoC commented May 3, 2016

Resolves #4645

Sets the unnamed register to the last-used one.

Adds a field is_unnamed to the ShaDa file for registers and sets the field to whatever register the unnamed register is set to.

Also added an option to setreg that sets the unnamed register to the set register (this was needed for testing).

Does this by finding the register with the most recent timestamp. If two registers have the same timestamp the first loaded one is used as the unnamed register.

I did it this way since I didn't really want to mess to much with the ShaDa file.

@marvim marvim added the RFC label May 3, 2016

@AdnoC AdnoC changed the title from [RFC] Keep unnamed register on restart to [WIP] Keep unnamed register on restart May 3, 2016

@marvim marvim added WIP and removed RFC labels May 3, 2016

@AdnoC AdnoC changed the title from [WIP] Keep unnamed register on restart to [RFC] Keep unnamed register on restart May 3, 2016

@marvim marvim added RFC and removed WIP labels May 3, 2016

@jamessan

This comment has been minimized.

Member

jamessan commented May 4, 2016

Looks good to me. I think @ZyX-I did a lot of the shada work, so his input would be useful.

@@ -1,6 +1,7 @@
-- ShaDa registers saving/reading support
local helpers = require('test.functional.helpers')
local nvim_command, funcs, eq = helpers.command, helpers.funcs, helpers.eq
local nvim_command, funcs, eq, execute = helpers.command, helpers.funcs,
helpers.eq, helpers.execute

This comment has been minimized.

@ZyX-I

ZyX-I May 4, 2016

Contributor

Remove execute. There is nvim_command, use it.

/// @param[in] name Register name.
///
/// @return true on success, false on failure.
bool op_previous_register_set(const char name)

This comment has been minimized.

@ZyX-I

ZyX-I May 4, 2016

Contributor

That is better named op_register_set_previous.

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented May 4, 2016

This actually changes the behaviour: Vim marks some register as the previous one (and so under some circumstances there may be no " set: e.g.

vim -u NONE -i /tmp/viminfo -N --cmd 'call setreg("1", ["abc"])' --cmd 'q!'

). This variant always sets " to something, and that something may be different from Vim. Variant that does the same thing that Vim does is something like

diff --git a/src/nvim/shada.c b/src/nvim/shada.c
index 32a02b0..6d4f2fd 100644
--- a/src/nvim/shada.c
+++ b/src/nvim/shada.c
@@ -103,6 +103,7 @@ KHASH_SET_INIT_STR(strset)
 #define REG_KEY_TYPE "rt"
 #define REG_KEY_WIDTH "rw"
 #define REG_KEY_CONTENTS "rc"
+#define REG_KEY_UNNAMED "ru"

 #define KEY_LNUM "l"
 #define KEY_COL "c"
@@ -287,6 +288,7 @@ typedef struct {
       char **contents;
       size_t contents_size;
       size_t width;
+      bool is_unnamed;
       dict_T *additional_data;
     } reg;
     struct global_var {
@@ -479,6 +481,7 @@ static const ShadaEntry sd_default_values[] = {
           .contents = NULL,
           .contents_size = 0,
           .width = 0,
+          .is_unnamed = false,
           .additional_data = NULL),
   DEF_SDE(Variable, global_var,
           .name = NULL,
@@ -1427,7 +1430,7 @@ static void shada_read(ShaDaReadDef *const sd_reader, const int flags)
           .y_width = (colnr_T) cur_entry.data.reg.width,
           .timestamp = cur_entry.timestamp,
           .additional_data = cur_entry.data.reg.additional_data,
-        })) {
+        }, cur_entry.data.reg.is_unnamed)) {
           shada_free_shada_entry(&cur_entry);
         }
         // Do not free shada entry: its allocated memory was saved above.
@@ -1868,6 +1871,7 @@ static ShaDaWriteResult shada_pack_entry(msgpack_packer *const packer,
           2  // Register contents and name
           + ONE_IF_NOT_DEFAULT(entry, reg.type)
           + ONE_IF_NOT_DEFAULT(entry, reg.width)
+          + ONE_IF_NOT_DEFAULT(entry, reg.is_unnamed)
           // Additional entries, if any:
           + (size_t) (entry.data.reg.additional_data == NULL
                       ? 0
@@ -1888,6 +1892,14 @@ static ShaDaWriteResult shada_pack_entry(msgpack_packer *const packer,
         PACK_STATIC_STR(REG_KEY_WIDTH);
         msgpack_pack_uint64(spacker, (uint64_t) entry.data.reg.width);
       }
+      if (!CHECK_DEFAULT(entry, reg.is_unnamed)) {
+        PACK_STATIC_STR(REG_KEY_UNNAMED);
+        if (entry.data.reg.is_unnamed) {
+          msgpack_pack_true(spacker);
+        } else {
+          msgpack_pack_false(spacker);
+        }
+      }
       DUMP_ADDITIONAL_DATA(entry.data.reg.additional_data, "register item");
       break;
     }
@@ -2741,7 +2753,8 @@ static ShaDaWriteResult shada_write(ShaDaWriteDef *const sd_writer,
     do {
       yankreg_T reg;
       char name = NUL;
-      reg_iter = op_register_iter(reg_iter, &name, &reg);
+      bool is_unnamed = false;
+      reg_iter = op_register_iter(reg_iter, &name, &reg, &is_unnamed);
       if (name == NUL) {
         break;
       }
@@ -2761,6 +2774,7 @@ static ShaDaWriteResult shada_write(ShaDaWriteDef *const sd_writer,
               .width = (size_t) (reg.y_type == MBLOCK ? reg.y_width : 0),
               .additional_data = reg.additional_data,
               .name = name,
+              .is_unnamed = is_unnamed,
             }
           }
         }
@@ -3769,6 +3783,7 @@ shada_read_next_item_start:
             entry->data.reg.contents[i] = BIN_CONVERTED(arr.ptr[i].via.bin);
           }
         }
+        BOOLEAN_KEY("register", REG_KEY_UNNAMED, entry->data.reg.is_unnamed)
         TYPED_KEY("register", REG_KEY_TYPE, "an unsigned integer",
                   entry->data.reg.type, POSITIVE_INTEGER, u64, TOU8)
         TYPED_KEY("register", KEY_NAME_CHAR, "an unsigned integer",
diff --git a/runtime/autoload/shada.vim b/runtime/autoload/shada.vim
index 9be85b6..e885e16 100644
--- a/runtime/autoload/shada.vim
+++ b/runtime/autoload/shada.vim
@@ -45,7 +45,7 @@ call map(copy(s:SHADA_ENTRY_NAMES),
 let s:SHADA_MAP_ENTRIES = {
   \'search_pattern': ['sp', 'sh', 'ss', 'sb', 'sm', 'sc', 'sl', 'se', 'so',
   \                   'su'],
-  \'register': ['n', 'rc', 'rw', 'rt'],
+  \'register': ['n', 'rc', 'rw', 'rt', 'ru'],
   \'global_mark': ['n', 'f', 'l', 'c'],
   \'local_mark': ['f', 'n', 'l', 'c'],
   \'jump': ['f', 'l', 'c'],
@@ -139,6 +139,7 @@ let s:SHADA_STANDARD_KEYS = {
   \'rt': ['type', 'regtype', s:SHADA_ENUMS.regtype.CHARACTERWISE],
   \'rw': ['block width', 'uint', 0],
   \'rc': ['contents', 'binarray', s:SHADA_REQUIRED],
+  \'ru': ['is unnamed', 'boolean', g:msgpack#false],
   \'n':  ['name', 'intchar', char2nr('"')],
   \'l':  ['line number', 'uint', 1],
   \'c':  ['column', 'uint', 0],

(I did not provide required changes to tests and ops.c).

@AdnoC AdnoC changed the title from [RFC] Keep unnamed register on restart to [WIP] Keep unnamed register on restart May 4, 2016

@AdnoC

This comment has been minimized.

Contributor

AdnoC commented May 4, 2016

Thanks @ZyX-I. I would never have figured out how to add that to the ShaDa file myself.

@marvim marvim added WIP and removed RFC labels May 4, 2016

@AdnoC AdnoC force-pushed the AdnoC:keep-default-register branch from 20a3979 to ca19270 May 4, 2016

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented May 4, 2016

Also needs documentation update: some file in runtime/doc contains complete ShaDa file description.

@@ -715,6 +722,7 @@ describe('In autoload/shada.vim', function()
' # Unexpected enum value: expected one of 0 (CHARACTERWISE), '
.. '1 (LINEWISE), 2 (BLOCKWISE)',
' + rt type 10',
' + ru is_unnamed FALSE',

This comment has been minimized.

@ZyX-I

ZyX-I May 4, 2016

Contributor

Note what tests are testing. Just adding this line to the failing tests is not enough.

This comment has been minimized.

@ZyX-I

ZyX-I May 4, 2016

Contributor

Specifically I remember that I check each and every possible key with unexpected type. Also a test where header contains every single key which is defined for any kind of entry. And a test where header contains every single key which is defined for any kind of entry, but with value of another type.

This comment has been minimized.

@ZyX-I

ZyX-I May 4, 2016

Contributor

Also there are register entry tests which a) contain each key value equal to default one and b) contain each key value not equal to default (but yet with a correct type).

This comment has been minimized.

@AdnoC

AdnoC May 25, 2017

Contributor

What exactly do I need to add for these tests? Is it that I need to test that it errors correctly for each of the non-boolean messagepack types? I only had one with an incorrect type (passed an int instead of a boolean), so should I add more?

I can't find the case where the values of each key are the default. Can you post the line number?

eq({{'zero'}, 'v'}, getreg('0'))
eq({{'one'}, 'v'}, getreg('1'))
eq({{'one'}, 'v'}, getreg('"'))
end)

This comment has been minimized.

@ZyX-I

ZyX-I May 4, 2016

Contributor

Missing test where unnamed register is not set and thus not restored.

This comment has been minimized.

@AdnoC

AdnoC May 29, 2017

Contributor

Isn't that what the test directly above it does?

This comment has been minimized.

@ZyX-I

ZyX-I May 29, 2017

Contributor

All tests here test that there something in unnamed register. This does not look like “not set”.

This comment has been minimized.

@AdnoC

AdnoC May 29, 2017

Contributor

This matches the behavior in Vim as far as I can tell. Doing:

call setreg('0', 'This is in reg 0', 'c')
echo getreg('"')
call setreg('3', 'This is in reg 3', 'c')
echo getreg('"')

Results in This is in reg 0 being printed twice.

While I haven't tested the hypothetical case where the viminfo/shada file has something in register 0 but no info on the unnamed register, that won't occur and make a difference unless someone manually edits the file. I have the feeling that the behavior should be the same.

This comment has been minimized.

@ZyX-I

ZyX-I May 29, 2017

Contributor

Just don’t do setreg('0'). setreg('3') does not set '"'.

This comment has been minimized.

@ZyX-I

ZyX-I May 29, 2017

Contributor

The point is that you check what would be done if there are some registers stored in a ShaDa file, but among them there is no one linked with unnamed register. This is achieved by setting '3' without setting anything else. Or setting 'a'. Better set both though.

This comment has been minimized.

@AdnoC

AdnoC May 29, 2017

Contributor

If the unnamed register isn't set and nothing is stored in register 0, it will be blank regardless of what the other registers are set to, just like Vim.

This comment has been minimized.

@AdnoC

AdnoC May 29, 2017

Contributor

Ah. You want a test to test that.

@@ -14755,6 +14757,9 @@ static void f_setreg(typval_T *argvars, typval_T *rettv)
--stropt;
}
break;
case 'u': case '"':

This comment has been minimized.

@ZyX-I

ZyX-I May 4, 2016

Contributor

Different cases should be on different lines.

@AdnoC AdnoC force-pushed the AdnoC:keep-default-register branch from ca19270 to 6ed10f2 May 6, 2016

@AdnoC AdnoC changed the title from [WIP] Keep unnamed register on restart to [RFC] Keep unnamed register on restart May 6, 2016

@marvim marvim added RFC and removed WIP labels May 6, 2016

@AdnoC

This comment has been minimized.

Contributor

AdnoC commented May 6, 2016

This fails Travis because of a linting error.

The shada_write function in shada.c is over 500 lines long. However, it was originally 499 lines; I only added 3 lines.

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented May 6, 2016

@AdnoC This means you should move something out. I have done this a few times, this is why it is close to 500.

@AdnoC AdnoC force-pushed the AdnoC:keep-default-register branch from 838acfb to 809be0f May 6, 2016

@AdnoC

This comment has been minimized.

Contributor

AdnoC commented May 6, 2016

I'm... not quite sure what to do with the log output of the failed Travis job (this one). It looks like test49.vim failed due to some VimL error. Because of something?

There aren't any problems when I run make inside of src/nvim/testdir (I'm running Mac OS X 10.11.4 and used gcc-5) and I don't really know how to debug the error in the Travis log.

Looks like it just needed to run the tests again.

@AdnoC AdnoC force-pushed the AdnoC:keep-default-register branch from 809be0f to b554beb May 10, 2016

@@ -14820,6 +14822,10 @@ static void f_setreg(typval_T *argvars, typval_T *rettv, FunPtr fptr)
}
break;
}
case 'u': case '"': { // unnamed register

This comment has been minimized.

@ZyX-I

ZyX-I May 29, 2017

Contributor

Two spaces before the comment or it would fail linter.


if (set_unnamed) {
if (!op_register_set_previous(regname)) {
EMSG("Unable to set previous yank register");

This comment has been minimized.

@ZyX-I

ZyX-I May 29, 2017

Contributor

And also note that if you have error you need a test where error is checked unless it is too hard (“too hard” mostly applies to FS errors and races). I have seen lots of bugs in error handling code just because it was not tested; some appeared after the refactoring which did not intend to change functionality.

This comment has been minimized.

@AdnoC

AdnoC May 29, 2017

Contributor

Thinking about this more, f_setreg already prints an error in the case where op_register_set_previous would fail (when regname isn't a valid name it errors with E354). So it would make sense to discard op_register_set_previous's result and not print a separate error message.

@@ -1800,6 +1803,14 @@ static ShaDaWriteResult shada_pack_entry(msgpack_packer *const packer,
PACK_STATIC_STR(REG_KEY_WIDTH);
msgpack_pack_uint64(spacker, (uint64_t) entry.data.reg.width);
}
if (!CHECK_DEFAULT(entry, reg.is_unnamed)) {

This comment has been minimized.

@ZyX-I

ZyX-I May 29, 2017

Contributor

You have not specified the default. Currently it works only because when using { .attr = value } syntax if some .attr is missing it is initialized with zeroes. Search for DEF_SDE(Register above.

This comment has been minimized.

@AdnoC

AdnoC May 29, 2017

Contributor

Added a default value.

.timestamp = reg.timestamp,
.data = {
.reg = {
.contents = (char **) reg.y_array,

This comment has been minimized.

@ZyX-I

ZyX-I May 29, 2017

Contributor

This would not pass linter either: need no space after a cast. Note that file was written before linter check in question was introduced and I was personally more fond of the space in most cases. (Also note that (PossiblyFreedShadaEntry) { above is not a cast, it needs not be changed.)

This comment has been minimized.

@AdnoC

AdnoC May 29, 2017

Contributor

That was copy-pasted from the body of another function, but I guess I might as well change it.

This comment has been minimized.

@AdnoC

AdnoC May 29, 2017

Contributor

It seems like it needs the (PossibyFreedShadaEntry) to use designated initialization. Removing gives a syntax error.

This comment has been minimized.

@ZyX-I

ZyX-I May 29, 2017

Contributor

I did not mean you need to remove it. I meant that due to it being not a cast you don’t need to remove a space.

This comment has been minimized.

@AdnoC

AdnoC May 31, 2017

Contributor

Ah

///
/// @param[in] wms The WriteMergerState used when writing.
/// @param[in] max_reg_lines The maximum number of register lines.
static void shada_initialize_registers(WriteMergerState *const wms,

This comment has been minimized.

@ZyX-I

ZyX-I May 29, 2017

Contributor

ALWAYS_INLINE needs inline here as well: static inline void.

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented May 29, 2017

Other then last comments LGTM.

@AdnoC

This comment has been minimized.

Contributor

AdnoC commented May 29, 2017

Oops. Forgot to copy something that changed in the copy-pasted section in shada_initialize_registers since I copied it.

@AdnoC AdnoC force-pushed the AdnoC:keep-default-register branch from 920eb0e to c502505 May 29, 2017

@AdnoC

This comment has been minimized.

Contributor

AdnoC commented May 29, 2017

Not sure why it would fail job spec tests.

@justinmk

This comment has been minimized.

Member

justinmk commented May 31, 2017

QB failure is unrelated. @AdnoC is this ready, or was there another test to be added?

@AdnoC

This comment has been minimized.

Contributor

AdnoC commented May 31, 2017

@justinmk It's good. I want to squash related commits together first though.

Once I do that I'll mark it as [RDY]. I'll do that once I get back from class in 2-3 hours.

AdnoC added some commits May 4, 2016

shada/linting: Moved some code out of shada_write.
shada_write was too long (over 500 lines) and caused a linting error.
Register initialization was moved to its own function in order to save lines.
test: Fix and add cases for unnamed register
Also:

Add ru to shada tests with all keys

Add test for unset unnamed and register 0
eval.c: Ignore unnamed register error in f_setreg
The error case is already handled and an appropriate error message is
already printed.

@AdnoC AdnoC force-pushed the AdnoC:keep-default-register branch from c502505 to 2f2eeb1 May 31, 2017

@AdnoC AdnoC changed the title from [RFC] Keep unnamed register on restart to [RDY] Keep unnamed register on restart May 31, 2017

@AdnoC

This comment has been minimized.

Contributor

AdnoC commented May 31, 2017

Good to go. I mostly left the branches from last-year untouched, but I tried to merge singe-change lint commits.

Before rebasing I made a branch which can be found here. Running git diff keep-default-register..keep-default-register-original showed there were no differences, so the rebase should have went well.

@justinmk

This comment has been minimized.

Member

justinmk commented May 31, 2017

I'll wait for @ZyX-I to change his review to "approved".

@ZyX-I

ZyX-I approved these changes May 31, 2017

@justinmk justinmk merged commit 133f8bc into neovim:master May 31, 2017

4 checks passed

QuickBuild Build pr-4700 finished with status SUCCESSFUL
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 75.833%
Details

@justinmk justinmk removed the RFC label May 31, 2017

@AdnoC

This comment has been minimized.

Contributor

AdnoC commented Jun 5, 2017

Woo! Glad this is finally finished.

justinmk added a commit to justinmk/neovim that referenced this pull request Nov 8, 2017

NVIM v0.2.1
FEATURES:
0e873a3 Lua(Jit) built-in neovim#4411
5b32bce Windows: `:terminal` neovim#7007
7b0ceb3 UI/API: externalize cmdline neovim#7173
b67f58b UI/API: externalize wildmenu neovim#7454
b23aa1c UI: 'winhighlight' neovim#6597
17531ed UI: command-line coloring (`:help input()-highlight`) neovim#6364
244a1f9 API: execute lua directly from the remote api neovim#6704
45626de API: `get_keymap()` neovim#6236
db99982 API: `nvim_get_hl_by_name()`, `nvim_get_hl_by_id()` neovim#7082
dc68538 menu_get() function neovim#6322
9db42d4 :cquit : take an error code argument neovim#7336
9cc185d job-control: serverstart(): support ipv6 neovim#6680
1b7a9bf job-control: sockopen() neovim#6594
6efe84a clipboard: fallback to tmux clipboard neovim#6894
6016ac2 clipboard: customize clipboard with `g:clipboard` neovim#6030
3a86dd5 ruby: override ruby host via `g:ruby_host_prog` neovim#6841
16cce1a debug: $NVIM_LOG_FILE neovim#6827
0cba3da `:checkhealth` built-in, validates $VIMRUNTIME neovim#7399

FIXES:
105d680 TUI: more terminals, improve scroll/resize neovim#6816
cb912a3 :terminal : handle F1-F12, other keys neovim#7241
619838f inccommand: improve performance neovim#6949
04b3c32 inccommand: Fix matches for zero-width neovim#7487
60b1e8a inccommand: multiline, other fixes neovim#7315
f1f7f3b inccommand: Ignore leading modifiers in the command neovim#6967
1551f71 inccommand: fix 'gdefault' lockup neovim#7262
6338199 API: bufhl: support creating new groups neovim#7414
541dde3 API: allow K_EVENT during operator-pending
8c732f7 terminal: adjust for 'number' neovim#7440
5bec946 UI: preserve wildmenu during jobs/events neovim#7110
c349083 UI: disable 'lazyredraw' during ui_refresh. neovim#6259
51808a2 send FocusGained/FocusLost event instead of pseudokey neovim#7221
133f8bc shada: preserve unnamed register on restart neovim#4700
1b70a1d shada: avoid assertion on corrupt shada file neovim#6958
9f534f3 mksession: Restore tab-local working directory neovim#6859
de1084f fix buf_write() crash neovim#7140
7f76986 syntax: register 'Normal' highlight group neovim#6973
6e7a8c3 RPC: close channel if stream was closed neovim#7081
85f3084 clipboard: disallow recursion; show hint only once neovim#7203
8d1ccb6 clipboard: performance, avoid weird edge-cases neovim#7193
01487d4 'titleold' neovim#7358
01e53a5 Windows: better path-handling, separator (slash) hygiene neovim#7349
0f2873c Windows: multibyte startup arguments neovim#7060

CHANGES:
9ff0cc7 :terminal : start in normal-mode neovim#6808
032b088 lower priority of 'cursorcolumn', 'colorcolumn' neovim#7364
2a3bcd1 RPC: Don't delay notifications when request is pending neovim#6544
023f67c :terminal : Do not change 'number', 'relativenumber' neovim#6796
1ef2d76 socket.c: Disable Nagle's algorithm on TCP sockets neovim#6915
6720fe2 help: `K` tries Vim help instead of manpage neovim#3104
7068370 help, man.vim: change "outline" map to `gO` neovim#7405

@justinmk justinmk referenced this pull request Nov 8, 2017

Merged

NVIM v0.2.1 #7505

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