search

hundredrabbits / Orca-c

Live Programming Environment(C Port)
http://wiki.xxiivv.com/orca
MIT License
482 stars 48 forks source link

added 8-jump #44

Closed a773music closed 5 years ago

a773music commented 5 years ago

ctrl+arrow moves 8 steps ctrl+shift+arrow expands selection 8 steps

npisanti commented 5 years ago

have you formatted the commit with clang-format?

a773music commented 5 years ago

I have no idea what that is...

npisanti commented 5 years ago

orca-c repo includes a clang-format file for formatting the code, it is required for all the commits otherwise new commits with contain diffs from nonformatted older commits, you should install it in your platform and run clang-format -i tui_main.c for example, you have to do it for all the file you edited

a773music commented 5 years ago

Sorry, this is my first pull request, I removed the stupid comment and formatted with clang-format and pushed them to my fork, should I make a new pull request or "can you see" the changes in this pull request?

npisanti commented 5 years ago

yes i can see them, but there are some strange differences, what clang-format version are you using? for example the default in debian package is old, i had to install clang-format-6.0

a773music commented 5 years ago

Got it, thanks! Great the fixes are coming through as I puss them. So additional changes I make (not related to 8-jump) should be in another branch of mine or?

npisanti commented 5 years ago

yes, to keep the commit separated there should be a PR for each topic, when merging the PR we will squash the commit together to a single commit for keeping it clean

( the best of the best would be for you to squash rebase your commits to a single one to make us easier to review the changes before merging )

cancel commented 5 years ago

What terminal did you test this with? It doesn't work with any of the ones I have.

npisanti commented 5 years ago

i just tested it on the following: st : not working xterm : working gnome-terminal : working terminator : CTRL move working but shift selection not working

cancel commented 5 years ago

I didn't test it with xterm because I'm on the road right now. But I don't think this worked in my old test environment. I'll have to try it when I get home next week.

a773music commented 5 years ago

What do you mean "not working"? What OS and version of OS + terminal? Is selection size modification "working" (shift arrow)?

npisanti commented 5 years ago

on my side i did my tests on debian stretch, i will elaborate better:

stterm : CTRL + arrow generates an A/B/C/D glyph and reduce x grid size, CTRL+SHIFT+arrows does nothing

terminator : CTRL + arrow works as intended, CTRL+SHIFT+arrows does nothing, it doesn't even move the cursor

xterm : working

gnome-terminal : working

neauoire commented 5 years ago

Works on OSX.

npisanti commented 5 years ago

@neauoire can you also test it on the rpi terminal? i have no access to that at the moment

neauoire commented 5 years ago

I can't I'm currently traveling, I get back on tuesday.

a773music commented 5 years ago

I leaned on the approach taken for some of the other key combos, like shift-arrow. The code mention that it's "These may not work in all terminals. (Only tested in xterm so far.)", so I guess that's what bit me.

I tried the proposed terminals + tty (I'm on debian stretch BTW), and can reproduce the problems. Poking around suggests there's something more going on here, for instance on stterm, ctrl+arrowdown seems to send four keycodes. Frankly I'm at a loss here, any suggestions more than welcome.

Another thing: in electron-orca the jump is in fact not always 8 positions, but follows the rulers, I need to make it work like this in orca-c, not hardcoding the 8-jump, but use the current ruler size...

npisanti commented 5 years ago

ok it seems that some modified keystroke are not registered in linux terminals, they are present just in terminals based on xterm, that's why stterm behaves differently

https://stackoverflow.com/questions/9061111/ncurses-with-arrow-keys-plus-modifiers

i'd be fine with that as long as those keystrokes also works on raspberry pis

npisanti commented 5 years ago

update: in debian by pressing CTRL+ALT+1/2/3/4/5/6/7/etc you can go to the default terminal you have without starting X11. In that terminal CTRL and SHIFT + Arrows does not works, even for default selection without CTRL.

@neauoire and @cancel what do you think we should do about this? keeping it, but warning about this feature working just on xterm-derivated terminals?

cancel commented 5 years ago

I think it's ok to keep. Please squash every commit together into a single commit, and use a commit message like:

Add ctrl+arrow keys for big step movement

Only works in some terminals.

Commit msgs should be worded as if they are a label of what action will occur to the code when applying the patch. (I think we should add that disclaimer and the clang-format requirement to the readme.)

Once that's done I'll review the commit again after that to make sure it's clean. Then we can use it as an example of how to contribute to orca-c in the future for other people. And then I don't have to review commits and make people wait on me as often :)

npisanti commented 5 years ago

@cancel do you mean squashing while merging the PR and then you review it later, or that @attejensen has to rebase all the commits and force push to make them into one so you can review and then we merge?

PS: i propose to have a CONTRIBUTING.md for instruction on how to contribute to orca-c.

cancel commented 5 years ago

Yes, rebase or just reset+recommit it into a single commit. Don't squash it when merging. The history is messy now and will make git blame and other things harder to use in the future. (This doesn't really matter for such a small project, but I'm trying to prevent it from decaying into a mess.)

a773music commented 5 years ago

Replaced by #45