-
Notifications
You must be signed in to change notification settings - Fork 13
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: transform the display #43
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good and the 180 flip is working.
TODO
- fixes mentioned in review
- Actually apply the Cfg value in
desire_transform
- Print the transformation information in
info.c
- Add transformation to CLI; merge etc. looks complete so I expect it should Just Work
src/cfg.cpp
Outdated
cfg_user_mode_free(user_transform); | ||
continue; | ||
} | ||
if (!parse_node_val_int(transform, "DEGREE", &user_transform->transform, "TRANSFORM", user_transform->name_desc)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need to be more strict about the value here: only a few values are applicable, and we need to account for flipped.
We can use a similar scheme to sway:
Can be one of "90", "180", "270" for rotation; or "FLIPPED", "FLIPPED-90", "FLIPPED-180", "FLIPPED-270" to apply a rotation and flip, or "NORMAL" to apply no transform.
convert.c methods similar to auto_scale_val
and auto_scale_name
will need to be created to perform that validation and conversion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need time to figure out this one. Not getting enough time from work :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no rush
- ability to specify transform value in terms of int - ability to hot reload - prints info value with transform info - accepts transform through cmd line (does not work - still broken)
Things left to do:
Thank you for waiting. Last month, I was super occupied at work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a few fixes. I don't have access you push to your repo, so here are some patches you can git apply ...
in order.
0001-formatting.patch.txt
0002-remove-head.tranform-always-use-desired-and-current.patch.txt
0003-assorted-fixes-for-transform-read-write.patch.txt
This is working quite nicely now.
Tested OK:
- value from cfg.yaml
- set / reset from CLI
- delete from CLI
- write from CLI
TODO:
- user friendly strings -> enums Feature: transform the display #43 (comment)
- doc: readme and man page
- consistency in naming: always use "transform", not "rotation"
src/cfg.cpp
Outdated
cfg_user_mode_free(user_transform); | ||
continue; | ||
} | ||
if (!parse_node_val_int(transform, "DEGREE", &user_transform->transform, "TRANSFORM", user_transform->name_desc)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no rush
src/layout.c
Outdated
if (!head->desired.enabled) | ||
return; | ||
|
||
// attempt to find a mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/nit "attempt to find a transform"
There is a large refactor incoming, which greatly affects cfg. I am happy to merge it to your repo. I'll need to have push access... applying a large merge as a patch is not viable. |
I have added you to the collaborators list. I hope now you will have the push access. |
Thanks @anandubey I'll merge master to your fork after https://github.com/alex-courtis/way-displays/tree/49-yaml-schemas is merged. |
master at 1.6.0 has been merged, along with the patches above. The large change resulted from refactors when adding the IPC API. A lot of src/cfg was moved to src/marshalling I've pre-emptively added !!transform to the YAML schema |
Nudge @anandubey Any updates on this one? This is really close to complete and I look forward to releasing it! |
@alex-courtis Sorry for zoning out on this. Started my new job on 1st Aug. It's a rust dev role, so all my weekends are going in that. I will try my best to finish it this week. |
No worries. There is no hurry at all. Good luck in the new role! |
Thanks for your work on this @anandubey, and totally understand about prioritizing paid job. It would be great to get output transforms in tho. Do you anticipate having time to get this merge ready going forward, or alternatively, would you be open to handing it off? If C was fresher in my mind I would volunteer (and my ADHD brain likes to say yes) but I shouldn't. |
That would be fantastic! I just cannot allocate the time to finish this one off...
way-displays is also set up for ccls live checking in the editor. Here's my config for neovim lsp: https://github.com/alex-courtis/arch/blob/bd011f38a006cecc2f32526b944d145e164dd7d2/config/nvim/lua/amc/plugins/lsp.lua#L11 |
cool!
alright! I use helix which uses clangd by default but I can change it in |
All good; clang is preferred and used by ccls. gcc is only used by default for legacy packaging reasons. |
Added development information to https://github.com/alex-courtis/way-displays/blob/master/CONTRIBUTING.md |
Want to try to get this in before #83? I would be willing to do a testing pass and make any needed changes if you have time for code review. I think I basically remember how to C :) I'll be on a road trip/camping Apr 14-23 though, so we'll see how much keyboard time I get. |
Oh yes please! I've added a lot of unit tests on master, so that should make this easier RE merge etc. Edit: Rather than trying to merge this branch it might be easier just to create a new branch and manually copy the relevant bits. Things have changed a lot since this was raised. |
Closes #41
@anandubey let's continue discussions in here.