Ticket #4597 (closed enhancement: fixed)

Opened 8 weeks ago

Last modified 4 weeks ago

Improve fish 4.0 shell support

Reported by: zaytsev Owned by: zaytsev
Priority: minor Milestone: 4.8.33
Component: mc-core Version: master
Keywords: Cc: aclopte@…
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Change History

comment:1 Changed 8 weeks ago by zaytsev

  • Description modified (diff)

comment:3 Changed 8 weeks ago by zaytsev

Yes, I don't like the environment variable thing all... Is it possible for fish to parse our version from mc --version? If this is not practical, maybe we could consider exporting a generic MC_VERSION variable?

comment:4 Changed 8 weeks ago by johannes

I just sent an updated patch series to the mailing list,
to use MC_VERSION instead.
That third patch is not absolutely necessary but it would be nice, to allow users to converge quicklier on the new keyboard input style.

Let me know if there is anything else for me to do; I realize I didn't add a changelog entry; not sure if that's necessary.

comment:5 Changed 7 weeks ago by zaytsev

Could you please just attach them here (git format-patch HEAD~~)? I'm not sure if there are any differences otherwise and it's really painful to save messages and upload. I will remove my first comment.

comment:6 Changed 7 weeks ago by zaytsev

  • Description modified (diff)

Changed 7 weeks ago by johannes

Changed 7 weeks ago by johannes

comment:7 Changed 7 weeks ago by johannes

Ok done.
Indeed git format-patch is exactly what git send-email sends, so running git am will give the same result.
I wasn't familiar with Trac, maybe the "Reporting problems" section in the README should say that it's preferred to submit patches on Trac.
(I also somehow didn't receive an email notification for comments)

comment:8 Changed 7 weeks ago by zaytsev

Thank you! I hope we solve the familiarity problem by dropping Trac one day... instead of adding more info to the README. But good point, need to check what it says now. Not very smart if it currently says to send patches to the mailing list, then ask people to post to Trac on the mailing list.

comment:9 Changed 7 weeks ago by johannes

I personally enjoy using mailing lists but I can see how they are not for everyone. There's an initial barrier to entry that most potential contributors won't cross. Some projects offer multiple avenues for sending patches (for example both Github and a mailing list), I think that's often a good choice

comment:10 Changed 6 weeks ago by johannes

Do you feel like there should be more thought spent on "MC_VERSION"?
If preferred, I can try parsing "mc --version" instead.

comment:11 Changed 6 weeks ago by zaytsev

Do you feel like there should be more thought spent on "MC_VERSION"?

I really don't like exporting it. It would be good to know what Andrew thinks about it...

Regarding lists, it's just an impractical way for us to keep track of patches right now. We need a central place, or things are bound to get lost or forgotten, as is currently the case with mailing lists and GitHub, and currently that central place is Trac. I hope that one day we'll get to GitHub and decommission everything else (Trac), but there's still a lot of work to be done to make that happen.

comment:12 Changed 6 weeks ago by ossi

i suggest emulating what shells do: set the version variable without exporting it. that would imply injecting an additional command early during the subshell setup.

comment:13 Changed 6 weeks ago by johannes

  • Cc aclopte@… added

comment:14 Changed 5 weeks ago by zaytsev

  • Owner set to zaytsev
  • Status changed from new to accepted
  • Branch state changed from no branch to on review

Branch: 4597_fish_csi
Initial changeset:0ea77d2ec71d42c28c28573794a4ac95bb3a912a

I like the second option, agree with Andrew's fixups.

Last edited 4 weeks ago by zaytsev (previous) (diff)

comment:15 Changed 5 weeks ago by andrew_b

  • Votes for changeset set to andrew_b
  • Branch state changed from on review to approved

There is no need to write "Ticket #XXXX:" in the 2nd, 3rd, etc. commits in the branch.

comment:16 Changed 5 weeks ago by johannes

I see, I didn't realize there will already be a merge commit to group commits

comment:17 Changed 5 weeks ago by zaytsev

I will clean up, rebase and merge as soon as CI is in.

comment:18 Changed 4 weeks ago by zaytsev

Rebased on current master so that this branch gets CI and cleaned up as promised.

comment:19 Changed 4 weeks ago by zaytsev

Hmmm, build on macOS breaks:

common.c:185:53: error: initializer element is not a compile-time constant
static const size_t subshell_switch_key_csi_u_len = strlen (subshell_switch_key_csi_u);
                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

comment:20 Changed 4 weeks ago by johannes

Could probably use sizeof(subshell_switch_key_csi_u) - 1 as a workaround.
Actually it seems better to get rid of this constant; it's used twice but one of those uses can be elided by switching from memcmp to strcmp (no other change should be necessary)

comment:21 Changed 4 weeks ago by zaytsev

I've pushed a fixup and it indeed fixed the CI! Actually, this sounds like a compiler bug to me. I can't reproduce it locally with clang 16, but it does fail on macOS inside GitHub. No other platform with clang is failing though (FreeBSD is fine)... oh well, if it works - so be it.

comment:22 Changed 4 weeks ago by zaytsev

  • Status changed from accepted to testing
  • Votes for changeset changed from andrew_b to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

Merged to master as fc3ad5322848dba0d6cc180bbea1d2176398f567 .

Thank you everybody!

comment:23 Changed 4 weeks ago by johannes

Thanks! The corresponding change has been merged to fish as well.
LMK if there are more fish-related issues; I don't really use mc myself yet but I'm happy to help.

comment:24 Changed 4 weeks ago by zaytsev

  • Status changed from testing to closed

Fantastic, whenever I stumble upon a FISH ticket while triaging, I'll put you on CC. It's always of great help to have someone in the know... Schönes Wochenende!

Note: See TracTickets for help on using tickets.