Bugs in the Customization API

Hi Allen,

I've been working on some rather involved customizations, essentially replacements for special emacs modes I used to use (or would have, had I invested some more time into learning them). I've come across some bugs in the 4coder API, as well as some (simple?) things that would make my time with this a whole lot easier.

  • Opening more than 8 Query_Bars at once crashes 4coder:
     1
     2
     3
     4
     5
     6
     7
     8
     9
    10
    11
    12
    CUSTOM_COMMAND_SIG(repro_query_bar_crash) {
        Query_Bar qb = {0};
        qb.prompt = make_lit_string("Prompt: ");
        qb.string = make_lit_string("Some text...");
        
        for (int i = 0; i < 9; ++i) {
            start_query_bar(app, &qb, 0);
        }
        
        // We won't even make it here
    }
      
    

    I assume this is because they were not intended to be used the way I did. I built my own fuzzy string matcher, and essentially tried to replace the interactive_open UI by opening and closing these as the user types. Limiting the number of files shown to 7 (leaving the 8th bar for input) solved the problem.
    This turned out to work so well that I used the same infrastructure to build other fuzzy lists, e.g. for selecting an arbitrary command to execute. What I really would have liked is to just have some function that displays the UI, but uses the string matcher and list of strings I provide.
  • directory_cd doesn't do anything unless rel_path is "..". I can't really provide repro code here, because the command is supposed to fail if the specified relative path does not exist, and I don't know the directory contents you will be testing with.
    (Edit: It just occured to me that this one is almost certainly platform dependent. All of this is on Windows 8.1)
  • This is the one that surprised me the most: using buffer_set_setting to set BufferSetting_MapID exhibits really strange behaviour: if the specified mapid is anything other than 0, mapid_global (0x01000000), or mapid_file (0x01000001), the call silently fails and the buffer's mapid will be set to mapid_file instead.
    Speaking of buffer settings, I would also really like to associate some arbitrary information with a buffer that can be retrieved later. For example, when opening a buffer in *hex* mode, I create a new buffer where the actual byte values will be pretty printed. Many commands interacting with the hex buffer need to also reference the source buffer (i.e. the one viewed through the hex buffer). This relation could be expressed by storing the source buffer's buffer_id with the hex buffer.
I know you have a lot on your plate; the feature requests here are mere suggestions, of course.

I'm still in the process of upgrading my customization code to the newest 4coder version (which I've been meaning to do for nearly half a year now!), and heavily improving on my old implementation of these bigger features. That's why I won't share the actual customization code yet - most of it isn't done yet.
That's just a matter of time, though. Hacking on 4coder has been my go-to programming activity ourside of work for the past week or so, and it is tremendous fun.

Thanks again for making such a fantastic tool!
Cheers

Edited by Tristan Dannenberg on
I'm glad you are at least having fun with it!

These are all pretty interesting issues you've brought up, so I'll break them down and address them one at a time.

1. Query Bars are intentionally limited to 8 per view because they were never meant as the long term GUI solution, but just a hack that unfortunately stayed in the code for too long. The idea of a more general GUI system, where you can specify a list of strings such as in the file list, as well as many other GUIs you might want, is a big part of what I have been thinking about behind the scenes. These problems will start getting solved in 4.1.0 and later builds.

2. This directory_cd thing is a bit surprising. I will dig into it a bit. A few things that might be going wrong are not using a full path for dir, or not using backslashes.

3. Hmm. I'm not sure what could be going wrong with the mapid. The default customization uses a custom command map for code files and that works, so I would need a repro case for this one.

As far as extra custom variables set on a buffer, that is a really good point. The sticky jump code had the same problem and I just implemented a hash table to map buffer ids to extra memory, but I don't want to force everyone else to write hash tables too... I'll have to think about the right way to solve that one for a bit.
I sort of expected you would want a repro case for that, but it really is as simple as it sounds. I too did think I was misusing the API somehow, but after stepping through the assembly for a bit, it seems that you loop over the list of all maps to verify that the specified mapid has been initialized, but somehow end up defaulting to mapid_file anyway?

I'm not sure, I suck at reading x86 assembly, and didn't want to step on your toes by reverse engineering too much.

Here is a full config that creates two command maps, with ID 0 and 1 respectively, and sets the *scratch* buffer to use mapid 0.

You can press the spacebar to run a command that should essentially just trigger an assert that a buffer_set_setting call did what it was supposed to do. You can also comment out said assert to verify that the map has not been set at all, and it's not just the local Buffer_Summary that's not getting updated. (The commands are bound properly, as far as I can tell by using inherit_map, though I don't show that here)

But maybe I AM misusing the API in some way and just going insane?

---

As for directory_cd, here is some code that is roughly equivalent to what I do already (which should work, if I read the documentation right):

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
CUSTOM_COMMAND_SIG(repro_cd) {
    char hot_dir_space[1024];
    String hot_dir = make_fixed_width_string(hot_dir_space);
    hot_dir.size = directory_get_hot(app, hot_dir.str, hot_dir.memory_size);
    
    File_List hot_files = get_file_list(app, expand_str(hot_dir));
    for (uint32_t i = 0; i < hot_files.count; ++i) {
        if (hot_files.infos[i].folder) {
            int32_t old_size = hot_dir.size;
            
            directory_cd(app, hot_dir.str, &hot_dir.size, hot_dir.memory_size, hot_files.infos[i].filename, hot_files.infos[i].filename_len);
            
            // This should crash also
            Assert(old_size != hot_dir.size);
            
            break;
        }
    }
}


(I do some more error checking in my own code, but as I said, it fails silently, for me anyways)

With that, thank you for all your efforts!

Edited by Tristan Dannenberg on
Okay I think this will work perfectly, I'll get these on my bug list and get back to you when I can.
Wow, a lot of time has passed where I couldn't really work on my customizations.

But since I've been sick all week, I decided to finally go back and finish what I needed before publishing the code. It can now be found on my GitHub, but there's still some cleanup left to be done, and probably some bugs left to be fixed. As always, there's also a lot of nice-to-haves I still haven't gotten around to (most of which can be found by searching the repository for TODO comments).

One of those is a git interface, which I started working on today, as well as mouse input support for the other command packs. Unfortunately, I can't really make progress on either because of more issues with the customization API.

First: exec_system_command. It works fine if you want to actually display the command line output to the user or run another graphical application, but I want to use the machine readable modes of the user-installed git executable, and parse the output immediately. This does not seem to be possible, as the command is executed asynchronously and there's no way to just wait for it to finish.

I'd like a separate function that works more like get_user_input (i.e. block the execution of the current command until the result is ready). While we're at it, it'd be great if that version just returns the status code of the executed command, rather than appending it to the text output.

Even with that, I don't see a good way to add support for git push and git pull, which may query the user for a name and password. If git accepted those as command line arguments, it'd be no problem, but that's apparently a bad idea for security reasons. The alternative is using SSH keys for authentication, but requiring that for the frontend to be functional doesn't seem great, though I can't really imagine what an API for sending user input to the other process should look like.

Speaking of get_user_input, it exhibits rather strange behaviour when called with the EventOnMouse flags:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
CUSTOM_COMMAND_SIG(repro_mouse_input_weirdness) {
    View_Summary active_view = get_active_view(app, AccessAll);
    View_Summary test_view = open_view(app, &active_view, ViewSplit_Bottom);
    
    while (true) {
        User_Input in = get_user_input(app, EventOnLeftButton, 0);
        
        if (in.abort) {
            // NOTE: This will be reached in the iteration after the mouse
            // button release event, even if we don't produce any more input,
            // though this seems to depend on which view you click in?
            Assert(false);
        } else if (in.type == UserInputMouse) {
            // Pretend we actually do something here
            continue;
        }
    }
    
    close_view(app, &test_view);
}


In the code where I stumbled across this issue, this actually causes a crash inside 4coder after the command has exited, though I can't seem to find a simple repro-case for that. I imagine this might be because I don't clean up properly, since I use a lot of temporary views in weird combinations with view_set_highlight.
Thanks for the update! I'll take this all into consideration. For now, I will just clarify how the current system is meant to work with get_user_input.

Even if you don't request an abort on any type of input, there is always a way to abort a command and that is by changing the active view by clicking on (and releasing on) a different view. Whether or not that behavior is intuitive usually depends on the user and what they are trying to implement. In a new iteration of the API, input would not exist on a per-view basis like it does now, and you would be able to have commands take mouse input across multiple views, but the current architecture of the system does not allow that.

Hope that helps in some way.
Ahh, I guess that makes sense.

I really only added mouse support for completeness before releasing my customizations, since I try to not actually use the mouse in my own workflow, so I never noticed that pattern. Looks like I can't actually do that for the find-and-replace GUI, then. At least for now.

Looking forward to the next builds!
Hey there! It's me again. I hope you don't mind if I dust this thread off?

Quite a bit of time has passed since 4coder November, and while I immediately started updating my customizations (and taking care of lots of TODOs in the process), other side projects, Opus Magnum, university and life in general kind of got in the way, which is why I'm only posting now.

First of all, I must say that I was a bit disappointed to find that neither directory_cd, nor buffer_set_setting were fixed. It's not that big a deal, since I have worked around both of them before, but since (I think?) you mentioned at some point during November that you had crossed everything off your list, I figured I'd bring those up again.

Second, today I chased down a bug in my find and replace commands into the buffer_seek_string_forward and buffer_seek_string_insensitive_forward functions, which reported a match where there was none, for a quite nasty reason; using the former as an example:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
static void
buffer_seek_string_forward(Application_Links *app, Buffer_Summary *buffer, int32_t pos, int32_t end, char *str, int32_t size, int32_t *result){
    char read_buffer[512];
    
    if (buffer->size > end){
        *result = buffer->size;
    }
    else{
        *result = end;
    }
    
    if (size > 0 && size <= sizeof(read_buffer)){
        if (buffer->exists){
            String read_str = make_fixed_width_string(read_buffer);
            String needle_str = make_string(str, size);
            char first_char = str[0];
            
            read_str.size = size;
            
            char chunk[1024];
            Stream_Chunk stream = {0};
            stream.max_end = end;
            
            if (init_stream_chunk(&stream, app, buffer, pos, chunk, sizeof(chunk))){
                int32_t still_looping = 1;
                do{
                    for(; pos < stream.end; ++pos){
                        char at_pos = stream.data[pos];
                        if (at_pos == first_char){
                            buffer_read_range(app, buffer, pos, pos+size, read_buffer);
                            if (match_ss(needle_str, read_str)){
                                *result = pos;
                                goto finished;
                            }
                        }
                    }
                    still_looping = forward_stream_chunk(&stream);
                }while (still_looping);
            }
        }
        
        if (end == 0){
            *result = buffer->size;
        }
        else{
            *result = end;
        }
        
        finished:;
    }
}


Basically, it comes down to the read_buffer not being cleared, the unconditional read_str.size = size in line 18, and the buffer_read_range call in line 30 not being checked for success, all conspiring to allow the match_ss call in line 31 to read uninitialized memory, which under specific circumstances (depending on what was called before buffer_seek_string, and the value of pos relative to buffer->size), will actually return true. (If you'd like, I'll sit down and figure out a good way to repro this tomorrow - I just wanted to get the report out quickly.)

Now, it's pretty easy to prevent this from happening by calling the function with end = buffer->size - needle_str->size, rather than just end = buffer->size, but that requirement is not documented anywhere, and even if it were, this mistake is just too easy to make, and too hard to track down, in my opinion. I'd recommend just placing an early return at the top of the function, something along the lines of

1
2
3
4
if (pos > end - size) {
    *result = end;
    return;
}


I don't wanna be all negative, though! One of the side projects that got me side-tracked was playing around with Go. Naturally, I used 4coder for this, and pretty quickly ended up annoyed with the lack of support for the language, so I quickly whipped up this language header:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
/******************************************************************************
Author: Tristan Dannenberg
Notice: No warranty is offered or implied; use this code at your own risk.
*******************************************************************************
LICENSE

This software is dual-licensed to the public domain and under the following
license: you are granted a perpetual, irrevocable license to copy, modify,
publish, and distribute this file as you see fit.
*******************************************************************************
This file provides a parse context for golang syntax highlighting.
******************************************************************************/

#if !defined(FTLD_LANGUAGE_GO_HPP)
#define FTLD_LANGUAGE_GO_HPP

static Parse_Context_ID tld_parse_context_language_golang;

#define PSAT(s, t) {s, sizeof(s)-1, t}
static void
tld_init_language_golang(Application_Links *app){
    if (tld_parse_context_language_golang != 0) return;
    
    Parser_String_And_Type kw[] = {
        PSAT("true",  CPP_TOKEN_BOOLEAN_CONSTANT),
        PSAT("false", CPP_TOKEN_BOOLEAN_CONSTANT),
        
        PSAT("chan",      CPP_TOKEN_KEY_OTHER),
        PSAT("const",     CPP_TOKEN_KEY_OTHER),
        PSAT("default",   CPP_TOKEN_KEY_OTHER),
        PSAT("func",      CPP_TOKEN_KEY_OTHER),
        PSAT("import",    CPP_TOKEN_KEY_OTHER),
        PSAT("interface", CPP_TOKEN_KEY_OTHER),
        PSAT("map",       CPP_TOKEN_KEY_OTHER),
        PSAT("nil",       CPP_TOKEN_KEY_OTHER),
        PSAT("package",   CPP_TOKEN_KEY_OTHER),
        PSAT("range",     CPP_TOKEN_KEY_OTHER),
        PSAT("return",    CPP_TOKEN_KEY_OTHER),
        PSAT("struct",    CPP_TOKEN_KEY_OTHER),
        PSAT("type",      CPP_TOKEN_KEY_OTHER),
        PSAT("var",       CPP_TOKEN_KEY_OTHER),
        
        PSAT("break",       CPP_TOKEN_KEY_CONTROL_FLOW),
        PSAT("continue",    CPP_TOKEN_KEY_CONTROL_FLOW),
        PSAT("case",        CPP_TOKEN_KEY_CONTROL_FLOW),
        PSAT("defer",       CPP_TOKEN_KEY_CONTROL_FLOW),
        PSAT("else",        CPP_TOKEN_KEY_CONTROL_FLOW),
        PSAT("fallthrough", CPP_TOKEN_KEY_CONTROL_FLOW),
        PSAT("for",         CPP_TOKEN_KEY_CONTROL_FLOW),
        PSAT("go",          CPP_TOKEN_KEY_CONTROL_FLOW),
        PSAT("goto",        CPP_TOKEN_KEY_CONTROL_FLOW),
        PSAT("if",          CPP_TOKEN_KEY_CONTROL_FLOW),
        PSAT("select",      CPP_TOKEN_KEY_CONTROL_FLOW),
        PSAT("switch",      CPP_TOKEN_KEY_CONTROL_FLOW),
    };
    
    tld_parse_context_language_golang = create_parse_context(app, kw, ArrayCount(kw), 0, 0);
}
#undef PSAT

#endif


Having syntax highlighting was nice, but I had to disable auto-indenting files on save, since 4coder_auto_indent.cpp gets confused with Go's lack of semicolons. I got fed up with having to manually indent code like a peasant just as quickly, though, so today I made some modifications to the indenter, both in the get_indentation_marks function:

1
2
3
4
5
6
7
8
// NOTE: This goes at the top of the function, with other initialization code
bool tld_golang_indentation = false;
Parse_Context_ID parse_context_id = 0;
buffer_get_setting(app, buffer, BufferSetting_ParserContext, (int32_t *) &parse_context_id);

if (parse_context_id == tld_parse_context_language_golang) {
    tld_golang_indentation = true;
}

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
// NOTE: This goes in the middle of the function, where `statement_complete` is determined
if (!has_prev_usable_token){
    statement_complete = true;
}
else{
    if (prev_usable_token.type == CPP_TOKEN_BRACKET_OPEN ||
        prev_usable_token.type == CPP_TOKEN_BRACE_OPEN ||
        prev_usable_token.type == CPP_TOKEN_BRACE_CLOSE ||
        prev_usable_token.type == CPP_TOKEN_SEMICOLON ||
        prev_usable_token.type == CPP_TOKEN_COLON ||
        prev_usable_token.type == CPP_TOKEN_COMMA ||
        // NOTE: TLD modifications begin here:
        (tld_golang_indentation && (
            prev_usable_token.type == CPP_TOKEN_BOOLEAN_CONSTANT ||
            prev_usable_token.type == CPP_TOKEN_STRING_CONSTANT ||
            prev_usable_token.type == CPP_TOKEN_IDENTIFIER ||
            prev_usable_token.type == CPP_TOKEN_INTEGER_CONSTANT ||
            prev_usable_token.type == CPP_TOKEN_FLOATING_CONSTANT ||
            prev_usable_token.type == CPP_TOKEN_CHARACTER_CONSTANT ||
            prev_usable_token.type == CPP_TOKEN_PARENTHESE_CLOSE ||
            prev_usable_token.type == CPP_TOKEN_BRACKET_CLOSE)))
            // NOTE: TLD modifications end here
    {
        statement_complete = true;
    }
}


I figured you might like to integrate that into the default customization layer. Not that this is entirely altruistic; these kinds of modifications are enormously annoying to maintain in the face of 4coder updates, and it'd be great if you'd audit them for me, since you wrote the indenter, and I didn't spend too much time actually reading your code. I did test this stuff, obviously, and it seems to work fine, but still.

I'll push my other customization code to Github when I'm done upgrading the final command pack. Until then, I'll just apologize for the wall of text and thank you for your efforts and attention.

Best regards~

Edited by Tristan Dannenberg on Reason: Realized the Go language file does not include the `nil` keyword.
Thanks for the update. I did look at these in November but I quickly realized that the cost to fixing vs benefit to the fix was not as good as other things I had to do. Thanks for reminding me to keep them on the to do list. Also thanks for the notes about the Go setup, I think that's a first for 4coder!
And I'm back, with a repro case for the buffer_seek_string bug:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
CUSTOM_COMMAND_SIG(repro_bfss_uninit_stack_mem_read)
CUSTOM_DOC("Reproduces a bug in buffer_seek_string_forward.")
{
    View_Summary view = get_active_view(app, AccessAll);
    Buffer_Summary buffer = get_buffer_by_name(app, literal("*test*"), AccessAll);
    if (!buffer.exists){
        buffer = create_buffer(app, literal("*test*"), BufferCreate_AlwaysNew);
        buffer_set_setting(app, &buffer, BufferSetting_Unimportant, true);
        buffer_set_setting(app, &buffer, BufferSetting_WrapLine, false);
    }
    else{
        buffer_send_end_signal(app, &buffer);
        buffer_replace_range(app, &buffer, 0, buffer.size, 0, 0);
    }
    
    buffer_replace_range(app, &buffer, 0, buffer.size, literal("ABCDEFD"));
    
    int32_t search_pos = 0;
    buffer_seek_string_forward(app, &buffer, search_pos, buffer.size, literal("DEF"), &search_pos);
    
    Assert(search_pos == 3);
    search_pos += 1; // Increment position so that we don't get the same match twice
    
    buffer_seek_string_forward(app, &buffer, search_pos, buffer.size, literal("DEF"), &search_pos);
    
    // No second match in the buffer, so this should not fire
    // ... but it does!
    Assert(search_pos >= buffer.size);
}


Also, I revised the code for Go support a little so that the Parse_Context and my modifications to 4coder_auto_indent.cpp have better cooperation. Specifically, the combination I posted will indent statements incorrectly that follow a nil literal, or other keywords that can be statements in their own right (such as break). This one should behave correctly, however.