Change slightly how x/X works
#2595
Conversation
| @@ -135,6 +148,8 @@ private: | |||
| void end_edition(); | |||
| int m_edition_level = 0; | |||
| size_t m_edition_timestamp = 0; | |||
| mutable bool m_line_mode = false; | |||
| mutable bool m_keep_line_mode = false; | |||
There was a problem hiding this comment.
Is using mutable really necessary here?
There was a problem hiding this comment.
It's kind of a hack/workaround that mawww threw at me, when I didn't know what to do. You can revert Is it a hack, or is it a solution? :D to see the problem, or just see the error in this commit: dpc@b8d3f08
Basically: now some functions are taking Context& some consts Context& and this big hashmap is complaining, even though it it is storing Context& in intself, so I don't get why it can't promote.
src/input_handler.cc
Outdated
| } | ||
| } | ||
|
|
||
| context().hooks().run_hook(Hook::NormalKey, key_to_str(key), context()); | ||
| if (enabled() and not transient) // The hook might have changed mode | ||
| m_idle_timer.set_next_date(Clock::now() + get_idle_timeout(context())); | ||
|
|
There was a problem hiding this comment.
You should remove this hunk
src/normal.cc
Outdated
| m_func(context, {0, params.reg}); | ||
| if (params.count > 1) { | ||
| context.post_movement_logic(); | ||
| } |
There was a problem hiding this comment.
The braces could be removed here, to follow the style of the rest of the code, and reduce the size of the patch. Same for the subsequent hunks.
There was a problem hiding this comment.
Gosh. I think it's a terrible idea, but not my project, not my rules. Fixing. :)
There was a problem hiding this comment.
Would need to be fixed in subsequent hunks as well, prior to merging.
src/normal.cc
Outdated
| if (params.count > 1) { | ||
| context.post_movement_logic(); | ||
| } | ||
| } while(--params.count > 0); |
There was a problem hiding this comment.
Space after a keyword, applies to the subsequent hunks as well.
src/selectors.cc
Outdated
| @@ -832,6 +839,9 @@ trim_partial_lines(const Context& context, const Selection& selection) | |||
| BufferCoord& to_line_start = anchor <= cursor ? anchor : cursor; | |||
| BufferCoord& to_line_end = anchor <= cursor ? cursor : anchor; | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
I don't think this new line is necessary, there's already one on line 841.
|
#2590 became a broad discussion about different ideas, let's make it super clear that this PR is about one thing: changing when @mawww would be interesting to hear your opinion about this particular proposal. |
|
I have fixed the style. I was editing this with kakoune, that I haven't configured for cpp yet, so there were some troubles and omissions. :) |
|
About several formatting suggestions mentioned in reviews. Just wondering, why not set up git hooks or add file with rules for formatting program (clang-format for example). It works for such projects as Linux, could work here. And kakoune supports formatting, so it feels natural to have project formatting rules. |
|
I must say I am not really in favor of that change, my main issue is that this de-facto introduces a 'submode' into the normal mode, and the behaviour is pretty fuzzy, as it is context dependent. Say if you do In general, I think introducing this additional state is confusing and creates a dangerous precedent. I would love to improve the ergonomics of |
|
@andreyorst Yeah, I was wondering the same thing. Rust spoiled me. @mawww I understand. In practice I don't hit this fuzziness with this branch, while I get supper annoyed by having inconsistent behavior on empty lines. In this one particular case you mention, should this state move to a different data structure that is shared between clients? Where it would be? I also agree that this is de-fact |
Would'nt that be even worse ? If you press |
|
I don't know the details of buffer sharing between clients, so I have no idea which one is better. The state of |
Discussion in #2590