Ticket #2452 (closed defect: fixed)

Opened 14 years ago

Last modified 7 weeks ago

Doesn't save "Verbose operation" file operation option

Reported by: nixtrian Owned by: zaytsev
Priority: major Milestone: 4.8.33
Component: mc-tty Version: master
Keywords: settings save Verbose operation removed Cc:
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

I launch mc, go into the "Options"/"Configuration" menu, put a marker on the "Verbose operation", then press save ("auto save setup" is marked). When I launch MC next time and go to "Options"/"Configuration" menu — there is no "Verbose operation" marker.

I tried to "rm -rf ~/.mc", there was no result.

$ LANG=C mc -V
GNU Midnight Commander 4.7.5-pre1-16-g97ae800
Built with GLib 2.26.1
Using the S-Lang library with terminfo database
With builtin Editor
With subshell support as default
With support for background operations
With mouse support on xterm
With internationalization support
With multiple codepages support
Virtual File Systems: cpiofs, tarfs, sfs, extfs, ftpfs, fish
Data types: char: 8; int: 32; long: 64; void *: 64; size_t: 64; off_t: 64;

Change History

comment:1 Changed 14 years ago by nixtrian

There is redetermination: it saves settings well, but doesn't load it next start and rewrites in to default next shutdown.

comment:2 follow-up: ↓ 3 Changed 14 years ago by andrew_b

What about other settings?

comment:3 in reply to: ↑ 2 ; follow-up: ↓ 4 Changed 14 years ago by nixtrian

Replying to andrew_b:

What about other settings?

Other settings (I tested few) saving and loading normally

I am also tried to turn off autosave, then edited the ~/.mc/ini, but "Verbose operation" wasn't loaded.

comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 14 years ago by andrew_b

Replying to nixtrian:

Replying to andrew_b:

What about other settings?

Other settings (I tested few) saving and loading normally

Looks like you terminal is slow. On slow terminal, the Verbose option is forced set to 0. This can occurs in 2 cases:
1) you launch mc with -s/--slow option;
2) S-Lang (or NCurses) determines that the output speed (the baud rate) of your terminal is less than 9600 bps.

comment:5 in reply to: ↑ 4 ; follow-up: ↓ 6 Changed 14 years ago by nixtrian

Replying to andrew_b:

Replying to nixtrian:

Replying to andrew_b:

What about other settings?

Other settings (I tested few) saving and loading normally

Looks like you terminal is slow. On slow terminal, the Verbose option is forced set to 0. This can occurs in 2 cases:
1) you launch mc with -s/--slow option;
2) S-Lang (or NCurses) determines that the output speed (the baud rate) of your terminal is less than 9600 bps.

I launch mc without -s/--slow option;
I use urxvt terminal with speed 4000000 baud (as "stty -a" says);

comment:6 in reply to: ↑ 5 Changed 14 years ago by nixtrian

I've downgraded urxvt now to 9.07 version. The MC works fine in it (saves and loads the subj option well). And stty says that terminal speed is 38400 bps. So maybe it is an urxvt bug.

comment:7 Changed 14 years ago by andrew_b

  • Status changed from new to closed
  • Keywords settings save Verbose operation removed
  • Version master deleted
  • Resolution set to invalid
  • Milestone 4.8 deleted

OK. Closed.

comment:8 Changed 14 years ago by nixtrian

  • Keywords settings save Verbose operation removed added
  • Status changed from closed to reopened
  • Version set to master
  • Resolution invalid deleted
  • Milestone set to 4.8

I think I should reopen this bug, cause I talked to urxvt developers and they confirmed the urxvt baudrate is 4000000.

I modified the midnight.c a bit (insert "printf("%i", tty_baudrate ());" before tty_baudrate check) to see what tty_baudrate returns and it returns "0" on the recent urxvt. So this is mc/slang bug (because "stty" returns the correct value (4000000) ).

comment:9 Changed 14 years ago by andrew_b

  • Status changed from reopened to closed
  • Resolution set to invalid

OK.

    404 int
    405 tty_baudrate (void)
    406 {
    407     return SLang_TT_Baud_Rate;
    408 }

Since MC gets baud rate directly from S-Lang, I guess, this is bug of S-Lang. Please send a bugreport to S-Lang developer.

Would you build MC with NCurses and show the result here? Thanks!

comment:10 Changed 14 years ago by andrew_b

  • Status changed from closed to reopened
  • Resolution invalid deleted

comment:11 follow-up: ↓ 12 Changed 14 years ago by vsu

Reported the problem and the patch for S-Lang to the slang-users-l mailing list:

http://mailman.jedsoft.org/pipermail/slang-users-l/2011/000722.html

Applying this patch to S-Lang fixes the mc+urxvt problem for me.

Version 0, edited 14 years ago by vsu (next)

comment:12 in reply to: ↑ 11 ; follow-up: ↓ 13 Changed 14 years ago by andrew_b

  • Status changed from reopened to closed
  • Component changed from mc-core to mc-tty
  • Version master deleted
  • Resolution set to invalid
  • Milestone 4.8 deleted

Cool! Thanks!

Actually, this is not mc bug. Closed.

comment:13 in reply to: ↑ 12 Changed 14 years ago by fairplay

  • Status changed from closed to reopened
  • Resolution invalid deleted

Replying to andrew_b:

Cool! Thanks!

Actually, this is not mc bug. Closed.

It's definitely a mc bug, cause you should not turn off the verbose mode automatically on slow terminals, but you should leave it as an option.

Last edited 14 years ago by fairplay (previous) (diff)

comment:14 Changed 13 years ago by c0da

  • Branch state set to no branch

+1 for disabling the automatic non-verbose mode
people with slow terminals should be able to add -s to their command line. there is no option to force verbose mode for those with urxvt (and maybe other terminals)

this problem also exists in the ncurses version of mc:

$ mc --version
GNU Midnight Commander 4.7.5.2
Built with GLib 2.28.8
Using the ncurses library
With builtin Editor
With subshell support as default
With support for background operations
With mouse support on xterm and Linux console
With support for X11 events
With internationalization support
With multiple codepages support
Virtual File Systems: cpiofs, tarfs, sfs, extfs, ext2undelfs, ftpfs, fish
Data types: char: 8; int: 32; long: 64; void *: 64; size_t: 64; off_t: 64;

$ set | grep TERM
COLORTERM=rxvt
TERM=xterm

$ stty
speed 4000000 baud; line = 0;

$ ncurses5-config --version
5.7.20081102

comment:15 Changed 13 years ago by c0da

gdb tells me that tty_baudrate () == -1
that is less than 9600 but a invalid value in my opinion, therefore this should not cause mc on modern systems to fallback to minimal functionality.

the issue can be solved with this patch. you have to enable verbose mode when it was disabled with a unpatched version.

diff --git a/src/filemanager/midnight.c b/src/filemanager/midnight.c
index c92fe53..2019ac8 100644
--- a/src/filemanager/midnight.c
+++ b/src/filemanager/midnight.c
@@ -833,8 +833,7 @@ setup_mc (void)
     if (mc_global.tty.use_subshell)
         add_select_channel (mc_global.tty.subshell_pty, load_prompt, 0);
 #endif /* !HAVE_SUBSHELL_SUPPORT */
-
-    if ((tty_baudrate () < 9600) || mc_global.tty.slow_terminal)
+    if ((tty_baudrate () > 0 && tty_baudrate () < 9600) || mc_global.tty.slow_terminal)
         verbose = 0;
 }

comment:16 Changed 13 years ago by andrew_b

  • Version set to master
  • Milestone set to Future Releases

comment:17 Changed 12 years ago by selecao

I found a solution:

stty 115200

just before starting mc (or somewhere in your scripts)

comment:18 Changed 2 months ago by zaytsev

  • Owner set to zaytsev
  • Status changed from reopened to accepted
  • Milestone changed from Future Releases to 4.8.33

Screen libraries can return -1 if baudrate cannot be determined:

https://github.com/mirror/ncurses/blob/87c2c84cbd2332d6d94b12a1dcaf12ad1a51a938/ncurses/tinfo/lib_baudrate.c#L207

I think it makes sense to apply a check for >0, because then we can't reliably determine the speed and it's better to let user decide.

comment:19 Changed 2 months ago by zaytsev

  • Branch state changed from no branch to on review

Branch: 2452_handle_baudrate_error
Changeset: 1ff8adc00593e20bd351d2b594cce1e7e8bd1bc1

Last edited 2 months ago by zaytsev (previous) (diff)

comment:20 Changed 2 months ago by andrew_b

Double call of tty_baudrate() is not very good.

Last edited 2 months ago by andrew_b (previous) (diff)

comment:21 follow-up: ↓ 22 Changed 2 months ago by zaytsev

Pushed a fixup.

I hope we can enable C99 one day, writing

const int baudrate = tty_baudrate ();

instead of

int baudrate;
baudrate = tty_baudrate ();

enables massive constification, can help reduce the scopes and makes code more understandable / less verbose.

I wonder if you know any big important reason holding us back?

comment:22 in reply to: ↑ 21 Changed 2 months ago by andrew_b

Replying to zaytsev:

Pushed a fixup.

https://midnight-commander.org/wiki/Hacking

Don't mix variable declaration and code, declare variable only in the beginning of block. 

Split variable declaration and code using one empty line. 

enables massive constification,

Constification of integer is pointless in almost all cases.

can help reduce the scopes

Scope reduce is irrelevant to constification.

and makes code more understandable / less verbose.

Likewise.

comment:23 Changed 2 months ago by zaytsev

https://midnight-commander.org/wiki/Hacking

I'm aware of that text, and that's why I'm asking you if you know why that rule was put in there. It just says don't do it, but it doesn't explain why.

My understanding is that this was done to support old broken pre-C99 compilers that do not support mixing code and declarations:

../../../src/filemanager/filemanager.c:857:15: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement]
  857 |     const int baudrate = tty_baudrate ();
      |               ^

However, newer standards allow this, and there is a reason for it: to make the code safer. So I was wondering if you knew of any other reasons why it should not be done. If not, then maybe we can at least start requiring -std=c99.

Constification of integer is pointless in almost all cases.

This has nothing to do with ints. This applies to any type:

mc_type_t foo;    // this forces to NOT make `foo` const, because
foo = get_foo();  // it's initialized on the next line with a function call

There are a lot of places in mc where we are forced to initialise variables with some garbage like 0 because a function call would be a statement and that's not allowed.

I agree that const int rarely saves lives, especially if you pay attention to compiler warnings to avoid things like if (variable = TRUE) { ... }, but it does make it easier to reason about the code, knowing that the value of the variable cannot change after assignment. It's even more important for other types.

Scope reduce is irrelevant to constification.

It's not relevant to consification, but it's very relevant to mixing code and declarations. Right now we have tons of huge functions like:

type1_t var11, var12, ...;
type2_t var21, var22, ...;

{
  {
    // block 1
    // var11, var12 only used here
  }
  {
    // block 2
    // var21, var22 only used here
  }
}

At the moment it's not possible to move variables to where they belong (reduce scopes), because that would mix code and declarations. On top of that, of course, you can make most of them const.

Refactoring big functions into small functions is good, but a first small step is to reduce scopes and join declaration & initialization.

Likewise.

Likewise what, you think that the following is bad?

  • Reducing scopes
  • Joining declaration and "real" initialization
  • Making variables const

comment:24 Changed 8 weeks ago by zaytsev

As discussed, I have:

  • Updated HACKING
  • Updated NEWS
  • Changed the build system

All platforms are green:

https://github.com/MidnightCommander/mc/actions/runs/11649942649

comment:25 Changed 8 weeks ago by andrew_b

'''This is right:'''
{{{
void
foo (const int iterations)
{
    int result;

    do_one_thing (iterations);
    do_something (result);
    ...
}
}}}

'''This is wrong:'''
{{{
void
foo (int iterations)
{
    do_one_thing (iterations);
    do_something (iterations);
    ...
 }
}}}

Unclear what's wrong. Do you mean

'''This is right:'''
{{{
void
foo (const int iterations)
{
    int result;

    result = do_one_thing (iterations);
    do_something (result);
    ...
}
}}}

'''This is wrong:'''
{{{
void
foo (int iterations)
{
    iterations = do_one_thing (iterations);
    do_something (iterations);
    ...
 }
}}}

comment:26 Changed 8 weeks ago by andrew_b

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

comment:27 Changed 7 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

comment:28 Changed 7 weeks ago by zaytsev

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.