0

Specs

  • OS: Windows 10
  • Programming Language: C++14
  • Compiler: MSVC 2019
  • IDE: CLion 2019.3.3

Code:

    #define WINVER 0x0500
    #include <windows.h>
    #include <string>
    
    void press_enter() {
        // This structure will be used to create the keyboard
        // input event.
        INPUT ip;
    
        // Set up a generic keyboard event.
        ip.type = INPUT_KEYBOARD;
        ip.ki.wScan = 0; // hardware scan code for key
        ip.ki.time = 0;
        ip.ki.dwExtraInfo = 0;
    
    
        // Press enter
        ip.ki.wVk = 0x0D;
        ip.ki.dwFlags = 0; // 0 for key press
        SendInput(1, &ip, sizeof(INPUT));
    
        // Release the key
        ip.ki.dwFlags = KEYEVENTF_KEYUP; // KEYEVENTF_KEYUP for key release
        SendInput(1, &ip, sizeof(INPUT));
        Sleep(25);
    
    }
    
    void press_keys(std::string& text_to_write) {
        // This structure will be used to create the keyboard
        // input event.
        INPUT ip;
    
        // Load current window's keyboardLayout
        HKL kbl = GetKeyboardLayout(0);
    
        // Set up a generic keyboard event.
        ip.type = INPUT_KEYBOARD;
        ip.ki.wScan = 0; // hardware scan code for key
        ip.ki.time = 0;
        ip.ki.dwExtraInfo = 0;
    
        for (char& c : text_to_write) {
            // Press the corresponding 'c' key
            ip.ki.wVk = VkKeyScanEx(c, kbl);; // virtual-key code for the "a" key
            ip.ki.dwFlags = 0; // 0 for key press
            SendInput(1, &ip, sizeof(INPUT));
    
            // Release the key
            ip.ki.dwFlags = KEYEVENTF_KEYUP; // KEYEVENTF_KEYUP for key release
            SendInput(1, &ip, sizeof(INPUT));
            Sleep(25);
        }
    }
    
    void give_100000(std::string& item) {
        for (int i = 0; i < 10; i++) {
            press_keys(item);
            Sleep(25);
            press_enter();
            Sleep(25);
            press_enter();
            Sleep(25);
        }
    }
    
    int main() {
    
        // Pause for 5 seconds.
        Sleep(5000);
    
        std::string lumber = "lumberjack";
        std::string food = "cheese steak jimmy's";
        std::string gold = "robin hood";
        std::string stone = "rock on";
    
        give_100000(lumber);
        give_100000(food);
        give_100000(gold);
        give_100000(stone);
    
        // Exit normally
        return 0;
    }

What this program does

I'm still very much a beginner in C++. I wrote this program as a little challenge and to practice my C++. It simulates keyboard presses, specifically to type cheats quickly, so that I get a bunch of resources in Age of Empires II.

The problem

This code works perfectly as is. It does what I want it to do. The thing is, there is repeated code inside both press_enter() and press_keys() functions, namely:

INPUT ip;

// Set up a generic keyboard event.
ip.type = INPUT_KEYBOARD;
ip.ki.wScan = 0; // hardware scan code for key
ip.ki.time = 0;
ip.ki.dwExtraInfo = 0;

So I wanted to fix this.

What I tried

I thought I could just bring that piece of code outside all functions (right below the #includes) and make them act as global variables, so that ip is accessible by all functions. But doing this made CLion complain, and compiling gave me a gigantic list of incomprehensible errors (I can post if needed). When I hover over any of the 4 lines that start with ip., CLion says: " Unknown type name 'ip' ". I don't understand this, since ip was declared literally 2 lines above.

What I'm looking for

As I said, I am still a beginner in C++, so I would really like to understand what this means, if I'm missing some fundamental concept, and a way to make it work without unnecessarily repeating code.

Community
  • 1
  • 1
Simon Vega-
  • 551
  • 4
  • 5
  • 3
    Put the common code into a separate function, then call that function where needed. For example, see the `configure_input()` function in [this question](https://stackoverflow.com/questions/59864053/). – Remy Lebeau Jan 22 '20 at 22:24
  • 2
    At first glance, it appears that your `press_enter` is essentially equivalent to `press_keys("\r");`... – Jerry Coffin Jan 22 '20 at 22:24
  • Note: You are writing some very platform specific code. If you ever want your code to work on different platforms, you my want to re-evaluate what you are doing. – Jesper Juhl Jan 22 '20 at 22:25
  • 2
    Variables can be global, but statements cannot. The line `ip.type = INPUT_KEYBOARD;`, for example, must be inside a function somewhere. The error message says "Unknown **type name** ip." Because the compiler thinks you're trying to declare a function or a variable, and declarations generally start with a type name. It sees `ip` and says "Well, the rules say that a type name needs to go here, but `ip` is not a type name." (It's a variable name.) – Raymond Chen Jan 22 '20 at 23:21
  • @RemyLebeau That is a valid option! I guess if I wanted to be nitpicky about performance (because the INPUT is being created on every loop) I'd have to create the INPUT object in main and the pass it to the functions. – Simon Vega- Jan 23 '20 at 00:28
  • @JerryCoffin I did not know that could work, I'll give it a shot, thanks. Still I'm glad I asked this question this way. I'm learning a lot! – Simon Vega- Jan 23 '20 at 00:29
  • @RaymondChen Thank you for your detailed, almost "ELI5" explanation. I now understand statements cannot be executed outside functions. Seems obvious now. That's the explanation I was looking for. I think the best solution, so that the INPUT object isn't created with each loop, is to create it inside main(), configure it, and then just pass it by reference to the functions that need it. Would that be the best option performance-wise? – Simon Vega- Jan 23 '20 at 00:37
  • Hi, if there is an answer does solve your issue, or you could add your own answer, please feel free to mark it for the people with same question. – Drake Wu Jan 23 '20 at 05:19
  • 2
    Performance is not an issue here. Not least because of those calls to `Sleep`! You need to concentrate on the basics before you get round to performance, in any case. And when you do, don't think you can predict performance, always measure. One big problem you have is that you call `SendInput` passing just a single event. You are meant to provide all the events in an array, so that they can be inserted atomically. That's the main reason for the existence of `SendInput`, given that `keybd_event` existed before it. Your chance to discover `std::vector`. – David Heffernan Jan 23 '20 at 10:39

3 Answers3

1

And as to why you got the error "Unknown Type" is because you are trying to run code where it can only be declared.

Within the function, you are running code and the place where you can change behavior of an initialized type.

user3389943
  • 147
  • 1
  • 7
  • Thanks to yours and @RaymondChen I now understand I can't run statements outside functions. Seems obvious now. Thanks – Simon Vega- Jan 23 '20 at 00:38
0

The thing is, there is repeated code inside both press_enter() and press_keys() functions

If you want to reuse the same INPUT instance, pass it by reference to the functions that needs it - try to stay away from global variables.

Also note that sending single keyboard events this way is discouraged. You could make a helper class to keep the information you only need to initialize once and add a function to send complete event sequences (key down/up) instead. It's then easy to extend with other functions for special scenarios.

Example:

#define WINVER 0x0500
#include <windows.h>
#include <string>
#include <vector>

struct KeyboardInput {
    KeyboardInput() :
        kbl{ GetKeyboardLayout(0) }
    {}

    // the basic function to send a number of keys at once
    void press_keys(const std::string& text_to_write) {
        // create a vector of INPUT structs, double the size of the string
        // to be able to send both key down and key up events
        std::vector<INPUT> INPUTs(text_to_write.size() * 2);
        SHORT wVk;

        // loop though all characters in the string and fill the INPUT structs
        size_t idx = 0;
        for(auto ch : text_to_write) {
            wVk = KeyScan(ch);

            // fill the INPUT struct with data
            fill_input(INPUTs[idx], wVk, 0); // KEYDOWN
            fill_input(INPUTs[idx + 1], wVk, KEYEVENTF_KEYUP);

            idx += 2;
        }

        // Call SendInput once for the complete string
        SendInput(static_cast<UINT>(INPUTs.size()), INPUTs.data(), sizeof(INPUT));
    }

    // a press_keys overload with a delay between keystrokes
    void press_keys(const std::string& text_to_write, DWORD delay_ms) {
        for (auto ch : text_to_write) {
            press_keys(std::string(1, ch));
            Sleep(delay_ms);
        }
    }

    // functions using the above
    void give_100000(const std::string& item) {
        press_keys(item + "\n\n", 250); // sleep 250 ms between each keystroke
    }
    void press_enter() {
        press_keys("\n");
    }
private:
    SHORT KeyScan(TCHAR ch) {
        return VkKeyScanEx(ch, kbl);
    }
    void fill_input(INPUT& i, SHORT wVk, DWORD dwFlags) {
        i.type = INPUT_KEYBOARD;
        i.ki.wVk = wVk;
        i.ki.dwFlags = dwFlags;
        i.ki.time = 0;
        i.ki.dwExtraInfo = 0;
    }

    HKL kbl;
};

int main() {
    Sleep(4000);

    std::vector<std::string> messages{
        "lumberjack",
        "cheese steak jimmy's",
        "robin hood",
        "rock on"
    };

    KeyboardInput ip;

    for (const std::string& msg : messages) {
        ip.give_100000(msg);
    }
} // Exit normally (return 0 is default)
Ted Lyngmo
  • 93,841
  • 5
  • 60
  • 108
  • @IInspectable Is it still too much of an anti-pattern? I just got an anonymous downvote so I don't know if it's because I'm proposing a bad practise or that it doesn't answer the question. – Ted Lyngmo Jan 23 '20 at 13:48
  • 1
    It's using `SendInput` as it was meant to be used anyway. I don't know about the down-vote. I don't see much of a reason for that. Still, I'm reluctant to up-vote. The OP seems to be struggling the basics. They are asking for a simple abstraction, factoring common logic into a function. Layering another abstraction on top of that is probably confusing the OP more than it helps. – IInspectable Jan 23 '20 at 15:48
  • @IInspectable Thanks. I'll then leave the answer up since it's using `SendInput` correctly - and OP might perhaps find it useful later, if not now. – Ted Lyngmo Jan 23 '20 at 16:37
  • 1
    Thank you @TedLyngmo. I will indeed look into your code to improve mine, and learn how the `SendInput` pattern works. – Simon Vega- Jan 23 '20 at 17:27
-1

So from what I've gathered here and on other sites, I was making the mistake of trying to run a statement outside of a function. There are other great suggestions here which I recommend anyone having a similar question as mine read them, but with the help of the person (Ted Burke) whose code I based mine on, I believe I achieved what I initially wanted to do. Ted recommended I declare ip globally and simultaneously initialize it, as such:

INPUT ip = {.type = INPUT_KEYBOARD, .ki.wScan = 0, .ki.time = 0, .ki.dwExtraInfo = 0};

I tried it but the compiler gave me errors, so I rewrote it to this:

(The compiler errors were confusing to me, so bonus points to the one who can explain why the following code worked while Ted's didn't)

TL;DR Solution

INPUT ip = {ip.type = INPUT_KEYBOARD,
            ip.ki.wScan = 0,
            static_cast<LONG>(ip.ki.time = 0),
            ip.ki.dwExtraInfo = 0};

The reason for that static_cast<LONG> is because CLion detected the following: "Non-constant-expression cannot be narrowed from type 'DWORD' (aka 'unsigned long') to 'LONG' (aka 'long') in initializer list".

I like this solution because this way, we won't need to instantiate an INPUT object for every single keystroke. I know in this case the performance gains both in time and memory will likely be negligible (maybe even thanks to the compiler optimizing my first code, it wasn't even necessary?), but from a logical standpoint, to me it was unnecessary to repeat the instantiation of an INPUT hundreds of times, when it could be instantiated only once.

I can see I might be using SendInput in not the best pattern, and that I could just use press_keys("\r") and get rid of press_enter(). I'll look into those changes to improve my code, but for now, my main concern has been fulfilled. Thank you all!

This is the final version of my code:

#define WINVER 0x0500
#include <windows.h>
#include <string>

INPUT ip = {ip.type = INPUT_KEYBOARD,
            ip.ki.wScan = 0,
            static_cast<LONG>(ip.ki.time = 0),
            ip.ki.dwExtraInfo = 0};
HKL kbl = GetKeyboardLayout(0);

void press_enter() {
    // Press enter
    ip.ki.wVk = 0x0D;
    ip.ki.dwFlags = 0; // 0 for key press
    SendInput(1, &ip, sizeof(INPUT));

    // Release the key
    ip.ki.dwFlags = KEYEVENTF_KEYUP; // KEYEVENTF_KEYUP for key release
    SendInput(1, &ip, sizeof(INPUT));
    Sleep(25);
}

void press_keys(std::string& text_to_write) {
    for (char& c : text_to_write) {
        // Press the corresponding 'c' key
        ip.ki.wVk = VkKeyScanEx(c, kbl);; // virtual-key code for the "a" key
        ip.ki.dwFlags = 0; // 0 for key press
        SendInput(1, &ip, sizeof(INPUT));

        // Release the key
        ip.ki.dwFlags = KEYEVENTF_KEYUP; // KEYEVENTF_KEYUP for key release
        SendInput(1, &ip, sizeof(INPUT));
        Sleep(25);
    }
}

void give_100000(std::string& item) {
    for (int i = 0; i < 10; i++) {
        press_keys(item);
        Sleep(25);
        press_enter();
        Sleep(25);
        press_enter();
        Sleep(25);
    }
}

int main() {
    // Pause for 5 seconds.
    Sleep(5000);

    std::string lumber = "lumberjack";
    std::string food = "cheese steak jimmy's";
    std::string gold = "robin hood";
    std::string stone = "rock on";

    give_100000(lumber);
    give_100000(food);
    give_100000(gold);
    give_100000(stone);

    // Exit normally
    return 0;
}
Simon Vega-
  • 551
  • 4
  • 5
  • This is poor. Global variables are not good. Performance is not an issue. Can you see those sleeps? Even without them initializing a struct is trivial. You are about to make a syscall! That swamps it immensely. But stop thinking about performance. Think about encapsulation. Also, as I explained in comments the big problem here is sending one event at a time. Send them in one atomic call. – David Heffernan Jan 23 '20 at 20:06
  • "_Ted recommended I declare `ip` globally_" - If you read my answer again, you'll find that to be the opposite of what I recommended :-) Recap: "_pass it by reference to the functions that needs it - try to stay away from global variables_". If you don't create a `class` that contains `INPUT` and functions to operate on it, declare `ip` in `main` and pass it by reference to the functions that needs it. – Ted Lyngmo Jan 24 '20 at 08:45