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

Feature request: Make the message location configurable #60

Closed
unsigned-enby opened this issue Nov 17, 2023 · 10 comments
Closed

Feature request: Make the message location configurable #60

unsigned-enby opened this issue Nov 17, 2023 · 10 comments

Comments

@unsigned-enby
Copy link

unsigned-enby commented Nov 17, 2023

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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

@project-repo
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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 pushed a commit that referenced this issue Jan 26, 2024
Functionality:
  * Add configuration to anchor position of messages (#60, 71 in Bugs.md).
  * Add output priorisation (#38, 68 in Bugs.md).
  * Print whether output is active in dump.

Bug Fixes:
  * Fix gamma control (72 in Bugs.md).
  * Add missing commands to example config.
  * Add message_config to dump output.

Example Scripts:
  * Add screenshot script.
  * Make example scripts standalone.

Documentation:
  * Escape html tags in config man page. (#68, 69 in Bugs.md)
  * Improve README.md (#62, 70 in Bugs.md).
  * Improve SECURITY.md.
  * Add CONTRIBUTING.md (#62, 70 in Bugs.md).
  * FAQ: Add Firefox screenshare instructions for wayland.
  * FAQ: Add wlroots downgrading instructions for workaround in case of temporary wlroots incompatibility.
  * Update copyright notices to 2024.

Development Tooling:
  * Add new gpg keys (signed by the old ones).
  * Add PR template.
  * Add dev-FAQ.
  * Remove now-unnecessary fanalyzer pragmas.
@project-repo
Copy link
Owner

This has been released with 2.3.0.

I am closing this issue.

Cheers
project-repo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants