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

implement move{left,up,right,down} following exchange* variants #59

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

tslilc
Copy link

@tslilc tslilc commented Oct 29, 2023

Hi all,

Thanks for this great project. I just picked this up and found that i was missing the move* bindings from ratpoison. At a glance i don't think i saw any obvious contribution/formatting guidelines, but please lmk if i missed something.

This PR implements move{left,up,down,right} commands for which i basically copied the code from exchange*.

@tslilc
Copy link
Author

tslilc commented Oct 29, 2023

apologies in advance if the indentation is not quite right, my editor was not able to guess the combination of tabs and spaces well (emacs + https://github.com/jscheid/dtrt-indent which is usually enough)

@project-repo
Copy link
Owner

Hi tslilc

Thanks so much for taking the time to write up a pull request! We will
be looking into this in the next few days and get back to you then.

Cheers
project-repo

@project-repo
Copy link
Owner

Hi tslilc

We gave your code a spin and it seems to work well - nice work!

However, I haven't quite been able to understand the intention yet:
When a view is moved away from a tile, what should be displayed on the
original tile? Should it be the background color or the next view which
would be displayable on this tile? I seem to be observe both behaviours,
depending on the layout. Also, which tile should be focussed after a
move?

On the administrative side:

  • Could you please add a DCO to your message as per our documentation.
    (This shows you are allowed to contribute and are fine with
    publication under MIT)
    https://github.com/project-repo/cagebreak#developer-certificate-of-origin-dco
  • Could you please make sure that your force push would not break our history if merged
    (if you're not sure here, don't worry about it, just let us know
    and we can figure something out on our end) and change
    your PR to our development branch instead of master.
  • If you would like to be credited in our Contributors section, please state a name or
    pseudonym for us to add there.

Regarding your comment on indentation: We use clang-format and the
.clang-format file of the cagebreak repository. (you may disregard this for now,
if it makes anything more difficult, we can do this later.)

cheers and thanks for contributing
project-repo

@tslilc
Copy link
Author

tslilc commented Nov 1, 2023

thanks for your feedback! Sorry for any trouble, sounds like this code has some poorly/un- defined behaviour. I typically don't really have time until the weekend to work on these sorts of things, i hope that's OK.

I'll be sure to look into your administrative things, sorry, i probably could have found all of this documentation lying around.

I hope to write back soon (once i've reminded myself what ratpoison does) and fixed/cleaned up the code (i missed writing manual entries too)

@tslilc tslilc changed the base branch from master to development November 1, 2023 19:58
@project-repo
Copy link
Owner

Hi tslil

That sounds like a good plan, feel free to get back to us once you've
had some time to look into the code again (if you explain your
intention, maybe we can also come up with a solution together).

Don't pressure yourself if you don't have time though, there really
isn't any hurry. The goal for this project is good code not a quick
release.

Regarding the man pages: Please don't worry about making a big effort to write man
pages. Simply describe clearly what the code is supposed to do and we will
write them for you. This way, we are sure that we understand correctly what
you intended and you can check to make sure that we have understood what
you are trying to tell us.

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

Successfully merging this pull request may close these issues.

2 participants