Closed Strahinja closed 1 year ago
@Strahinja thank you for your work. could you please explain a little more what this PR achieves/solves? Thanks!
@Strahinja thank you for your work. could you please explain a little more what this PR achieves/solves? Thanks!
As stated in #778, and per your request, I made this PR to fix the issue that the variable (now renamed to import_delimited_to_text
) has no effect when a file is loaded by giving the pathname to sc-im on the command line. (Using the command :load
to load the file from within sc-im respects the variable.) The override:
sc-im --import_delimited_to_text myfile.tsv
works, but simply passing the pathname without the --import_delimited_to_text
switch, while having
set import_delimited_to_text=1
in the configuration file, doesn't work. Since the order of load_file
and load_rc
is swapped, the check
if (session->cur_doc == NULL) create_empty_wb();
is redundant, so I also removed it. The function create_empty_wb
is always called.
@Strahinja yes! sorry. my memory is so bad. I remember now. Will merge this today.
@Strahinja I have noticed if you open a file ./sc-im ../examples/sc/sheets/a.sc it creates a new wb even though its not needed. please see screenshot below.
My apologies, I haven't noticed that. You can revert the PR as a quick fix.
I still think the logical solution should be to move the configuration loading code before the loading of the file, since there are configuration variables affecting the loading. The part regarding the NULL check can be reverted, but IMHO the configuration loading code should be made independent of the file loading code and remain before the file loading code.
The move you made is ok. it should be kept as it is now. the only thing to be done is check for the null..
The thing is, in my testing sc-im segfaults with:
Program received signal SIGSEGV, Segmentation fault.
0x000055555556777c in readfile (fname=0x7fffffffbde0 "/home/strajder/.config/sc-im/scimrc", eraseflg=0) at file.c:742
742 char * curfile = roman->name;
(gdb) bt
#0 0x000055555556777c in readfile (fname=0x7fffffffbde0 "/home/strajder/.config/sc-im/scimrc", eraseflg=0) at file.c:742
#1 0x00005555555650a6 in load_rc () at file.c:156
#2 0x00005555555887b2 in main (argc=2, argv=0x7fffffffe688) at main.c:268
when loading a file in this situation:
if (session->cur_doc == NULL) create_empty_wb();
load_rc();
if (strlen(loadingfile))
load_file(loadingfile);
or this:
load_rc();
if (session->cur_doc == NULL) create_empty_wb();
if (strlen(loadingfile))
load_file(loadingfile);
or this:
load_rc();
if (strlen(loadingfile))
load_file(loadingfile);
if (session->cur_doc == NULL) create_empty_wb();
which leads me to conclude that load_rc
depends on structures allocated in create_empty_wb
(or load_file
). That's why I unconditionally called that function. I think the solution would be to either initialize the offending structures, or eliminate the code which relies on them in load_rc
.
Please update to latest commit on dev branch and check it out.
With my fork rebased to dev branch and with some of the changes reverted, the setting doesn't seem to be obeyed at all, both with specifying pathnames and :load
. My $HOME/.config/sc-im/scimrc
:
set import_delimited_to_text=1
@Strahinja this was ok: Why did you revert it? I have also changed read_file function in file.c Please check you have the same version as current dev branch in here: https://github.com/andmarti1424/sc-im/commit/b1ba526d0be4b4ceff13b4d89ebfcaba954d26a3
Ok, I pulled the latest dev, rebased my fork and reintroduced the changes I made again, meaning the code in src/main.c
reads:
Also, I have:
but after make -C src clean all
it seems that the configuration setting/variable import_delimited_to_text
is still not obeyed, meaning I got this:
Yes. Something seems wrong. :(
I got this. Found the problem.
The configuration variable name was wrong in file.c. Please update to latest commit on dev branch, or just replace import_delimited_as_text to import_delimited_to_text in file.c
Yes, I can confirm that works!
This PR rearranges the order of
load_rc
andload_file
and also removes some related redundant checks.Related issue: #778