reported by thread-sanitizer.
the sighandler's spoiled `errno` was causing xlib to incorrectly assume some
error occurred and thus causing the crash described in #391.
to reproduce:
* Open an nsxiv window
* Open another terminal and run the following:
var=$(pidof nsxiv); while :; do kill -s SIGCHLD $var; done
putting the `pid` into a variable is actually important because doing
`$(pidof nsxiv)` inside the loop makes it really hard to reproduce the
issue, I presume because of the extra process invocation it was sending
less SIGCHLD and so putting it into a variable avoids that overhead and
is able to generate more signals.
instead of reaping the zombies manually, we now pass the
`SA_NOCLDSTOP|SA_NOCLDWAIT` for SIGCHLD instead so that the zombies are
reaped automatically.
Closes: https://codeberg.org/nsxiv/nsxiv/issues/391
Reviewed-on: https://codeberg.org/nsxiv/nsxiv/pulls/411
Reviewed-by: explosion-mental <explosion-mental@noreply.codeberg.org>
posix_spawn() is designed especially for this purpose, and thus it's
much more lightweight and efficient than manually fork/dup/exec-ing.
on my system, it improves the performance of spawn() by about 10x. given
that we make frequent calls to potentially multiple scripts, the
increased efficiency will add up overtime.
using posix_spawn() also simplifies the logic quite a bit, despite the
very verbose function names. however it does make cleanup a bit more
complicated.
this patch uses the linux kernel style cleanup strategy [0] (which I'm
personally not a huge fan of, but it fits this situation quite nicely)
with a "stack-like" unwinding via `goto`-s.
additionally simplify the spawn() API by taking in {read,write}fd
pointers and returning the pid instead of using some custom struct.
this coincidently also fixes#299
[0]: https://www.kernel.org/doc/html/v4.10/process/coding-style.html?highlight=goto#centralized-exiting-of-functions
* Imlib2 supports modifying gamma, brightness and contrast directly
while sxiv only supports gamma. Makes sense to extend it to brightness
and contrast as well.
* Since color corrections need to be aware of each other, they have been
refactored into one centralized function.
* This also makes the code more hackable as it makes it easier to add
more color correction functions without them interfering with each
other.
Co-authored-by: 0ion9 <finticemo@gmail.com>
Co-authored-by: NRK <nrk@disroot.org>
Reviewed-on: https://codeberg.org/nsxiv/nsxiv/pulls/396
Reviewed-by: NRK <nrk@disroot.org>
Reviewed-by: TAAPArthur <taaparthur@noreply.codeberg.org>
Co-authored-by: Berke Kocaoğlu <kberke@metu.edu.tr>
Co-committed-by: Berke Kocaoğlu <kberke@metu.edu.tr>
this basically just extracts the logic that was previously inside
`main()` into a seperate function `add_entry()` so that it can be used
for accepting entries form stdin as well.
Closes: https://codeberg.org/nsxiv/nsxiv/issues/382
Reviewed-on: https://codeberg.org/nsxiv/nsxiv/pulls/383
Reviewed-by: Berke Kocaoğlu <berke.kocaoglu@metu.edu.tr>
* ensure static variables comes after non-static ones
* remove depreciated DATA32 type
* prefer `sizeof(expression)` over `sizeof(Type)`.
* silence a -Wsign warning
* {gif,webp} loader: use a pointer to reduce code-noise
* gif loader: allocate in one place
Reviewed-on: https://codeberg.org/nsxiv/nsxiv/pulls/374
Reviewed-by: TAAPArthur <taaparthur@noreply.codeberg.org>
* run_key_handler: make the logic easier to follow
* remove timeout_t
the typedef is not needed. inline the declaration similar to the other
static structs.
* simplify estrdup
reuse emalloc, instead of calling malloc and null-checking.
* win_clear: initialize `e` right away
* process_bindings: explicitly check against NULL
most pointer checks in the codebase do explicit check.
* use a named constant instead of magic number
also changes the padding from 3 to 4 bytes according to [0]. but i
couldn't find any situtation where this mattered, so perhaps the current
padding is enough. but doesn't hurt adding one more byte.
[0]: https://nullprogram.com/blog/2017/10/06/
Reviewed-on: https://codeberg.org/nsxiv/nsxiv/pulls/356
Reviewed-by: explosion-mental <explosion-mental@noreply.codeberg.org>
the current code is quite hacky and complex as it mixes multiple pointers. all
of this complexity is unnecessary. drop it by introducing an explicit scratch
buffer instead of implicitly abusing `arl->filename` as one. this also reduces
some unnecessary allocation overhead.
additionally, the argument to arl_setup must be the result of `realpath(3)` (as
commented in `nsxiv.h`). instead of commenting it, assert it.
and lastly, rename `arl_setup` to `arl_add` since it's not doing any "setup"
but rather *adding* a file to watch.
Reviewed-on: https://codeberg.org/nsxiv/nsxiv/pulls/342
Reviewed-by: explosion-mental <explosion-mental@noreply.codeberg.org>
this was one of the cases which got missed in 591be8c, if someone starts
with `-t` the statusbar will remain at "Loading ...". Once we're done
loading all the thumbnail, make sure to open_info() so that `thumb-info`
gets called.
Reviewed-on: https://codeberg.org/nsxiv/nsxiv/pulls/341
Reviewed-by: explosion-mental <explosion-mental@noreply.codeberg.org>
mixing signed and unsigned types in comparison can end up having
unintended results. for example:
if (-1 < 1U)
printf("true\n");
else
printf("false\n");
previously we silenced these warnings, instead just fix them properly
via necessary casting, and in cases where the value cannot be negative
(e.g width/height members) make them unsigned.
Reviewed-on: https://codeberg.org/nsxiv/nsxiv/pulls/336
Reviewed-by: explosion-mental <explosion-mental@noreply.codeberg.org>
* includes are sorted alphabetically
* their grouping and layout is the following:
- nsxiv.h will be the first include
- followed by any internal headers (e.g "commands.h" "config.h")
- followed by system headers (<stdlib.h> etc)
- followed by third party headers (X.h libwebp etc)
* also add `llvm-include-order` check to clang-tidy so that it can catch
unsorted includes during CI.
* rm unused include <sys/types.h>
* move <sys/time.h> to main.c, it's the only file that needs it.
* move TV_* macros to main.c
* let *.c files explicitly include what they need instead of including
them at nsxiv.h
it's possible for the close() calls to override the errno resulting in
incorrect error message being printed.
call error() immediately to avoid such possibilities.
also refactor a couple conditions to avoid doing multiple checks.
Reviewed-on: https://codeberg.org/nsxiv/nsxiv/pulls/321
Reviewed-by: Berke Kocaoğlu <berke.kocaoglu@metu.edu.tr>
instead of dancing around with some `init` parameter, directly give
`win_set_title()` what it needs. `get_win_title()` now also does *just*
what the name says.
this simplifies things quite a bit and the functions now do what their
name implies more closely instead of doing some `init` dance internally.
rather than calling the script unconditionally per redraw, we now have
a `title_dirty` flag and keep track of when any of the relavent
information changes.
Co-authored-by: Arthur Williams <taaparthur@gmail.com>
Partially fixes: https://github.com/nsxiv/nsxiv/issues/258
currently the way check_timeout() is implemented, animate has higher
priority than slideshow. so if doing a redraw takes longer than the
frame delay of the animated image then it's going to continuously keep
animating and never reliably get to slideshow.
this issue can occur if the animated image has too fast of a delay or if
nsxiv is being run on a slow system where doing redraw takes too long.
the issue can be emulated by artificially slowing down img_render by
sticking a sleep in there.
diff --git a/main.c b/main.c
index 5dc52d4..0580011 100644
--- a/main.c
+++ b/main.c
@@ -441,6 +441,7 @@ void redraw(void)
if (mode == MODE_IMAGE) {
img_render(&img);
+ nanosleep(&(struct timespec){0, 62000000}, NULL); /* 62ms */
if (img.ss.on) {
t = img.ss.delay * 100;
if (img.multi.cnt > 0 && img.multi.animate)
make it so that slideshow has higher priority than animate to fix
the issue.
Closes: https://github.com/nsxiv/nsxiv/issues/281
to reproduce:
1. have an image-info script
2. invoke the key-handler
3. cancle invocation by pressing `escape`
at this point, the statusbar ends up being empty.
the regression seems to be caused by 6922d5d (changing select to poll),
unsure why that is.
in any case, this simplifies read_info quite a bit and solves the
regression as well. in short:
* read straight into the statusbar buffer
* if read succeeds, make sure buffer is null terminated and replace any
newline with space
* close the script
this happens when the keyhandler is invoked while viewing an animated
image. if {image,thumb}-info scripts exists, everything works as
expected. but if they don't, then update_info will override the
statusbar.
before we were using select, which expected `struct timeval` as
arg. so we needed to do ms -> timeval conversions.
but now since we're using poll, which accepts milisec as arg, there's
no need to do ms -> timeval -> ms. instead have check_timeouts directly
return ms.
with a couple exceptions as they cause too many -Wshadow warnings.
also moves the `extcmd_t` typedef on top for cosmetic purposes.
also enable `-Wmissing-prototypes` in the ci
usage of select (3) in modern programs is typically discouraged.
this simply replaces the select call with poll (3) instead.
and since poll conveniently ignores negative fds, this also reduces
needs for some special casing.
this also handles error if they occur, while old implementation didn't.
other than the error handling, no change in functionality should occur.
this allows for configuring thumbnail mode mouse bindings similar to
image mode bindings.
however we can't put the thumbnails bindings into the existing buttons[]
array due to fallthrough. For example M3 would switch mode and then end
up selecting an image.
which is why thumbnail bindings have been put into it's own array
`buttons_tns[]` and `buttons[]` has been renamed to `buttons_img[]` for
consistency.
Closes: https://github.com/nsxiv/nsxiv/issues/131
fixes all -Wshadow related warnings (on gcc). this would allow us to use
`-Wshadow` in github workflow (https://github.com/nsxiv/nsxiv/pull/195).
i've thought about adding `-Wshadow` to our Makefile as well, but
decided against it to keep the Makefile CFLAGS barebore/minimal.
currently if the keyhandler invocation fails, for example due to it not
being present, the statusbar does not reset and stays on "getting
keyhandler input" message.
now the return value from run_key_handler() is used to determine if the
function was successful or not. and if the function failed, we call
handle_key_handler() with false which resets the statusbar.
we also no longer call redraw() within run_key_handler() and instead assign
it's return value to dirty which does a redraw if true.
rendering is a pretty expensive operation, especially when scaling with
anti-aliasing. by waiting for the image to render before setting
timeout, the actual timeout ends up being (render time + actual delay).
this pretty much fixes the slowdown entirely on all the images i've
tested. it should also improve things noticeably even in cases where
the delay between frames is shorter than how fast we can render.
although on such images, the issue may not be fixed entirely.
Closes: https://github.com/nsxiv/nsxiv/issues/70
* specifies the function argument type in commands.h compared to leaving
it unspecified. all the functions in cmd_t must have arg_t as it's
argument.
* changes to commands.h will now trigger a rebuild - this restores old
behavior prior to 12efa0e
* cg_quit now uses it's argument as exit status
* DestroyNotify invokes cg_quit rather than calling exit directly.
* Explicitly pass EXIT_SUCCESS to cgquit in keybinding
Co-authored-by: Berke Kocaoğlu <berke.kocaoglu@metu.edu.tr>
* fix: send implicit_mod to process_bindings
this solves the edge case where someone might have `ShiftMask + A` in
their keybindings compared to a plain `A`.
Closes: https://github.com/nsxiv/nsxiv/pull/166#issuecomment-978853136
* code-style: smuggle small style fix in
win_draw_bar now mimics autoreload_nop.c functions
with the exception of arrays, all other var names in config.h are in ALL
CAPS. since keyhandler_abort is an unreleased feature, it should be okay
to rename it for consistency.
though.. in the future we should be more careful about naming when
adding new vars to config.h (or the codebase in general.)
the goal here to mark functions and variables not used outside the
translation unit as static. main reason for this is cleanliness. however
as a side-effect this can help compilers optimize better as it now has
guarantee that a certain function won't be called outside of that
translation unit.
one other side-effect of this is that accessing these vars/function from
config.h is now different.
if one wants to access a static var/func from different translation unit
in config.h, he would have to create a wrapper function under the right
ifdef. for static functions one would also need to forward declare it.
here's a dummy example of accessing the function `run_key_handler` from
config.h under _MAPPINGS_CONFIG
```
static void run_key_handler(const char *, unsigned);
bool send_with_ctrl(arg_t key) {
run_key_handler(XKeysymToString(key), ControlMask);
return false;
}
```
while i was initially against this since it can be done via `xargs -0`.
the problem with this approach is that there's a limit to how many args
a command can recieve, leading to problem like this [0] when opening
large (1k~) amount of images.
there's no limit on how big stdin can be, so being able to read a
null-separated list from stdin doesn't have this problem.
[0]: https://github.com/ranger/ranger/pull/2307#issuecomment-818683515
this allows users to configure navigation width from config.h. it also
allows disabling the navigation function entirely by using a 0 width.
one extra functionality this adds is being able to define an absolute
width (in pixels) instead of just percentage via `NAV_IS_REL`.
Co-authored-by: NRK <nrk@disroot.org>
with this change `-0` is turned into a more generic switch which can be
used to send NULL-separated file-list to the key-handler as well.
this also means `-0` no longer implicitly enables `-o`
Closes: https://github.com/nsxiv/nsxiv/issues/140
NRK
Berke Kocaoğlu
explosion-mental
N-R-K
LuXu