Skip to content
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

Message "stream" #770

Closed
wants to merge 20 commits into from
Closed

Message "stream" #770

wants to merge 20 commits into from

Conversation

wizard7377
Copy link
Contributor

Here I've added a feature where you can create both messages from channels and direct messages from users with a cluster object using a "stream like" interface

@netlify
Copy link

netlify bot commented Aug 11, 2023

Deploy Preview for dpp-dev ready!

Name Link
🔨 Latest commit 41b7855
🔍 Latest deploy log https://app.netlify.com/sites/dpp-dev/deploys/64d7be9e1c87e9000726b723
😎 Deploy Preview https://deploy-preview-770--dpp-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@wizard7377
Copy link
Contributor Author

wizard7377 commented Aug 11, 2023

Example program:

#include <dpp/dpp.h>

int main()
{
    dpp::cluster bot("TOKEN");
 
    bot.on_log(dpp::utility::cout_logger());
 
   
    bot.on_slashcommand([&bot](const dpp::slashcommand_t & event) {
        /* Check which command they ran */
        if (event.command.get_command_name() == "rep") {
            dpp::channel_stream(bot,event.command.get_channel()) << "Test one" << dpp::component().add_component(
                        dpp::component().set_label("Click me!").
                        set_type(dpp::cot_button).
                        set_emoji(u8"😄").
                        set_style(dpp::cos_danger).
                        set_id("myid")
                    ) << dpp::end_msg() << "Test two" << dpp::end_msg();
            dpp::channel_stream(bot,event.command.get_channel().id) << "Test three" << dpp::end_msg();
            dpp::dm_stream(bot,event.command.get_issuing_user()) << "Test 4" << dpp::end_msg();         

            dpp::dm_stream(bot,event.command.get_issuing_user().id) << "Test 5" << dpp::end_msg();         
            
        }
    });
 
    bot.on_ready([&bot](const dpp::ready_t & event) {
        if (dpp::run_once<struct register_bot_commands>()) {
 
            /* Create a new global command on ready event */
            dpp::slashcommand newcommand("rep", "test", bot.me.id);
            
 
            /* Register the command */
            bot.global_command_create(newcommand);
        }
    });
 
    bot.start(dpp::st_wait);
 
    return 0;
}

src/dpp/streams.cpp Outdated Show resolved Hide resolved
src/dpp/streams.cpp Outdated Show resolved Hide resolved
src/dpp/streams.cpp Outdated Show resolved Hide resolved
include/dpp/streams.h Outdated Show resolved Hide resolved
Copy link
Contributor

@Jaskowicz1 Jaskowicz1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does base_stream& operator<<(const int& n); use & for the int (see streams.h)? ints shouldn't be passed by a const ref (see this for more info) as far as I'm aware.

cmake_install.cmake Outdated Show resolved Hide resolved
include/dpp/channel.h Outdated Show resolved Hide resolved
include/dpp/cluster.h Show resolved Hide resolved
include/dpp/dpp.h Outdated Show resolved Hide resolved
include/dpp/streams.h Outdated Show resolved Hide resolved
include/dpp/streams.h Outdated Show resolved Hide resolved
include/dpp/user.h Outdated Show resolved Hide resolved
library/CMakeLists.txt Show resolved Hide resolved
src/dpp/streams.cpp Outdated Show resolved Hide resolved
src/dpp/streams.cpp Outdated Show resolved Hide resolved
@Mishura4
Copy link
Member

Mishura4 commented Aug 12, 2023

Though this not explicitly mentioned in the coding standard, functions in DPP in general have their braces start on the same line, and have a newline in between definitions, e.g.

void foo {
	// stuff
}

void bar {
	// other stuff
}

Though I can't explicitly require that you change your code to fit that since it's not in the coding standard, it would be appreciated.

@Jaskowicz1
Copy link
Contributor

Though this not explicitly mentioned in the coding standard, functions in DPP in general have their braces start on the same line, and have a newline in between definitions, e.g.

void foo {
	// stuff
}

void bar {
	// other stuff
}

Though I can't explicitly require that you change your code to fit that since it's not in the coding standard, it would be appreciated.

This is mentioned in the coding standard https://dpp.dev/coding-standards.html

include/dpp/streams.h Outdated Show resolved Hide resolved
include/dpp/streams.h Outdated Show resolved Hide resolved
include/dpp/streams.h Outdated Show resolved Hide resolved
@wizard7377
Copy link
Contributor Author

Here's a new and improved test with the new features:

#include <dpp/dpp.h>

int main()
{
    dpp::cluster bot("TOKEN");
 
    bot.on_log(dpp::utility::cout_logger());
 
   
    bot.on_slashcommand([&bot](const dpp::slashcommand_t & event) {
        /* Check which command they ran */
        if (event.command.get_command_name() == "rep") {
            dpp::channel_stream(bot,event.command.get_channel()) << "Test 0" << dpp::end_msg() << "Test 1"
                    << dpp::component().set_type(dpp::cot_selectmenu).set_placeholder("Pick something").add_select_option(dpp::select_option("1","2","3")).set_id("6") 
					<< dpp::component().set_label("hello").set_type(dpp::cot_button).set_id("1").set_style(dpp::cos_primary)  
					<< dpp::component().set_label("hello2").set_type(dpp::cot_button).set_id("2").set_style(dpp::cos_primary)    
					<< dpp::end_row()
					<< dpp::component().set_label("hello3").set_type(dpp::cot_button).set_id("3").set_style(dpp::cos_primary)   
					<< dpp::end_msg();
            dpp::channel_stream(bot,event.command.get_channel().id) << "Test 2" << dpp::end_msg();
            
            dpp::dm_stream(bot,event.command.get_issuing_user()) << "Test 3" << dpp::end_msg();         

            dpp::dm_stream(bot,event.command.get_issuing_user().id) << "Test 4" << dpp::end_msg();         
            
        }
    });
 
    bot.on_ready([&bot](const dpp::ready_t & event) {
        if (dpp::run_once<struct register_bot_commands>()) {
 
            /* Create a new global command on ready event */
            dpp::slashcommand newcommand("rep", "test", bot.me.id);
            
 
            /* Register the command */
            bot.global_command_create(newcommand);
        }
    });
 
    bot.start(dpp::st_wait);
 
    return 0;
}

@Jaskowicz1
Copy link
Contributor

I would love to see what this looks like in discord itself. Whilst I like this new example with adding components, I don't like that dpp::end_msg() is used mid-way. I would interpret that as the message is now done but I'm assuming it means \n. Either way, seeing it do more than just send message and actually simplify adding a component does make me like this. I feel this would benefit newer people.

@wizard7377
Copy link
Contributor Author

I would love to see what this looks like in discord itself. Whilst I like this new example with adding components, I don't like that dpp::end_msg() is used mid-way. I would interpret that as the message is now done but I'm assuming it means \n. Either way, seeing it do more than just send message and actually simplify adding a component does make me like this. I feel this would benefit newer people.

It does indeed mean that the message is done

@Jaskowicz1
Copy link
Contributor

Jaskowicz1 commented Aug 12, 2023

I would love to see what this looks like in discord itself. Whilst I like this new example with adding components, I don't like that dpp::end_msg() is used mid-way. I would interpret that as the message is now done but I'm assuming it means \n. Either way, seeing it do more than just send message and actually simplify adding a component does make me like this. I feel this would benefit newer people.

It does indeed mean that the message is done

Okay, so going back to your example's first message. It shows dpp::channel_stream(bot,event.command.get_channel()) << "Test 0" << dpp::end_msg() << "Test 1" << dpp::component().set_type(dpp::cot_selectmenu).set_placeholder("Pick something").add_select_option(dpp::select_option("1","2","3")).set_id("6") << dpp::component().set_label("hello").set_type(dpp::cot_button).set_id("1").set_style(dpp::cos_primary) << dpp::component().set_label("hello2").set_type(dpp::cot_button).set_id("2").set_style(dpp::cos_primary) << dpp::end_row() << dpp::component().set_label("hello3").set_type(dpp::cot_button).set_id("3").set_style(dpp::cos_primary) << dpp::end_msg(); this, does this mean that it will send two separate messages (one being "Test 0", the other being "Test 1" with buttons)?

If so, I feel that this shouldn't be allowed as it becomes really weird looking, allowing more messages after an end message call. I guess you could also just not do it but still feels weird!

@Mishura4
Copy link
Member

Mishura4 commented Aug 12, 2023

I don't really like the function model still. It's harder for the compiler to inline than a constexpr variable, it doesn't look like std::cout << "foo" << std::endl;, also as a standalone dpp::end_msg() is confusing, what message is being ended? The name "end_msg" doesn't communicate that it's being sent very well. To me it sounds more like finishing a builder, which is something that does not usually have side effects. Also why provide one function for a message, what if we add streams for other facilities, will each of them need a separate function?

@wizard7377
Copy link
Contributor Author

wizard7377 commented Aug 12, 2023

Here are the images of the test code working
#general _ Trash 7 - Discord 8_12_2023 12_24_11 PM
#general _ Trash 7 - Discord 8_12_2023 12_24_27 PM

@wizard7377
Copy link
Contributor Author

I don't really like the function model still. It's harder for the compiler to inline than a constexpr variable, it doesn't look like std::cout << "foo" << std::endl;, also as a standalone dpp::end_msg() is confusing, what message is being ended? The name "end_msg" doesn't communicate that it's being sent very well. To me it sounds more like finishing a builder. Also why provide one function for a message, what if we add streams for other facilities, will each of them need a separate function?

What do you mean, if we add streams for other facilities?

@Mishura4
Copy link
Member

For example if you want to stream a thread and << users to it, to add them to the thread. Making streams work with other stream than messages.

@wizard7377
Copy link
Contributor Author

why provide one function for a message, what if we add streams for other facilities, will each of them need a

I don't get your point, are you saying that there are too many << overloads or too few?

@Mishura4
Copy link
Member

Mishura4 commented Aug 12, 2023

I'm not saying either of those.

What i'm saying is, when designing a feature that works on a large library, one needs to take into account how it interacts with the rest of the library too. If you can stream a channel and add messages to it to simplify calling cluster methods, then it makes sense that other cluster calls could be simplified with other things too. For example adding users to a thread. Then, because you chose end_msg as a name for a message, you'll have to add another function for that, end_user_add? Why not just unify calling conventions?

In fact now the more I think about it, the more i think it would make sense to have a message builder with your additions, that you can then stream to the channel stream you get on a channel.

dpp::message m = dpp::message::stream() << "Message text" << some_embed << component1 << component2 << stream::build;

channel.stream(cluster) << m << dpp::message{"another message"} << stream::send;
thread.stream(cluster) << dpp::message{"a message"} << user1 << user2 << stream::send; // sends the message and adds user1 and user2 to the thread

for example.

@Commandserver
Copy link
Member

I'm closing this PR as we see no benefit in this and it doesn't fit with the style of the lib.

@Commandserver Commandserver added the invalid This doesn't seem right label Aug 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants