project-repo / cagebreak

Cagebreak: A Wayland Tiling Compositor Inspired by Ratpoison
MIT License
279 stars 20 forks source link

Feature request: Make the message location configurable #60

Closed unsigned-enby closed 9 months ago

unsigned-enby commented 11 months ago

First off, let me say thank you for the work you have put in to cagebreak! While river is my 'main' WM, cagebreak provides a distraction free environment almost on par with the framebuffer. The one issue I've had is that messages are displayed in the top right of the screen. This isn't a 'major' issue persay, but my right eye is basically useless (ambliopia) so it's not entirely ideal. While changing it to the top left is as simple as ...

--- a/message.c 2023-11-17 16:05:19.556061285 -0600
+++ b/message.c 2023-11-17 16:08:44.706721504 -0600
@@ -266,12 +266,12 @@
    wlr_output_layout_get_box(output->server->output_layout, output->wlr_output,
                              &output_box);

-   box->x = output_box.width;
+   box->x = 0;
    box->y = 0;
    box->width = 0;
    box->height = 0;

-   message_set_output(output, buffer, box, CG_MESSAGE_TOP_RIGHT);
+   message_set_output(output, buffer, box, CG_MESSAGE_TOP_LEFT);
    free(buffer);
    alarm(output->server->message_config.display_time);
 }

I feel it would make sense to just use the cg_message_align enum to set both the anchor point (i.e screen relative position) and the alignment (i.e message box relative position), as their respective matches aren't likely to differ(e.g if you want the message in a corner of the screen, you're probably not going to want it centered on the message). My thinking would be to 'attach' the configured enum to the cg_message_config structure, alter message_printf as needed, but keep message_set_output (and message_printf_pos) the same. I'm likely to do this for my own benifit, but I'd be more than willing to submit a pull request, along with any particular preferences of yours, if you would like to add it into cagebreak!

project-repo commented 11 months ago

Hi unsigned-enby

Thank you for your feature request. We think it is a great idea to make Cagebreak more accessible. We will release this addition as soon as we find the time to implement and test it (modulo other features).

If you'd want to take the initiative of implementing something of the sort and submitting a pull request, that would certainly be great and speed things up. In this case, I think we'd want to have this as an option for "configure_message", as you suggest. Our proposed syntax would be configure_message position top_left (and similarly, bottom_right, etc., i.e., it would be nice to have the "bottom" options too). What do you think?

cheers project-repo

unsigned-enby commented 11 months ago

Hi project-repo,

Thanks for your timely response. I just finished up implimenting this change. I went with the term anchor since 1: 'position' is already the name of a wlr_box variable in message_set_output and 2: anchor conforms with other notification utilities (at least with mako that is). However I don't think it would be confusing to change anchor to position for the user-side of things (i.e in cagebreak/config & ipc-socket).

Just as well I added in a CG_MESSAGE_NOPT enum into cg_message_align (which is now cg_message_anchor) for the sake of keybinding_configure_message & parse_message_config. Since it's an enum, setting it to -1 isn't a (good?) option, but if I'm mistaken, or if there is a better way to do that in general, I would certainly like to hear your thoughts!

Lastly, I included the ability to set the bottom options as you suggested as well as top_center , bottom_center, and center for completeness. I tested this all on both my rock 5 (aarch64) and x86_64 laptop and it all works as expected!

Best, Luca

project-repo commented 11 months ago

Hi unsigned-enby

Thanks so much! Initial tests look spot on, great job! We'll do some further testing and create a feature branch for this asap (probably it'll be the weekend though before we get the necessary time), so that this can be included in the next release (which will again happen when we can find the time to get everything ready). In the meantime, it would be great if you could add a DCO to your pull request. Here is the relevant documentation: https://github.com/project-repo/cagebreak#developer-certificate-of-origin-dco (This shows you are allowed to contribute and are fine with publication under MIT). Regarding the naming convention, I think I agree that changing "anchor" to "position" on the user side of things would be a good idea, to keep consistent with e.g. ouput position. But we can change that once we merge your changes and have made a final decision. In any case, once again thanks a lot!

Cheers project-repo

unsigned-enby commented 11 months ago

Sounds good / DCO is now added to the pull request.

project-repo commented 11 months ago

Hi unsigned-enby, A few days ago, a new version of wlroots was released. It seems that making cagebreak compatible with this new version will be trickier than was a priori assumed... Unfortunately, that means it may take another week or so for us to get everything up and running to a degree that we are able to take a closer look at your PR, sorry about that. We'll get back to you then. Cheers, project-repo PS: Sorry, this should have been sent yesterday but was unfortunately delayed due to internal comunication issues.

unsigned-enby commented 11 months ago

That's fine! Thank you for letting me know. Wether it's merged or not I appreciate your responsiveness and communication. This has been my first official pull request and you have certainly helped make it a less nerve-racking experience. Best, unsigned-enby

project-repo commented 11 months ago

Hi unsigned-enby

Sorry for taking so long to get back to you. Wlroots 0.17 and other year-end deadlines got in the way of looking at this more closely. We've now done some testing and your code seems to work as advertised in all special cases, great job!

One thing we noticed though is that there appears to be some inconsistency in the documentation, i.e. at one point you use "align" and at another you use "anchor". Am I right in assuming that it should be "anchor" everywhere?

Our current plan is to do a year-end release which will include your code along with some other things that are still pending (e.g. we might get the "permanent" versus "peripheral" output setup to float). Does that work for you?

Cheers project-repo

unsigned-enby commented 10 months ago

No worries! With the end of the semester I haven't had much time myself to check for a response. As for the align vs anchor, anchor is certainly the intended/desired term. And that works for me. Best, Unsigned-enby

project-repo commented 10 months ago

Hi unsigned-enby

This has now been merged onto the development branch. Thanks again for putting in the effort to implement this!

Cheers project-repo

project-repo commented 9 months ago

This has been released with 2.3.0.

I am closing this issue.

Cheers project-repo