Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove notcurses_resize() from the public API #367

Closed
dankamongmen opened this issue Feb 21, 2020 · 7 comments
Closed

Remove notcurses_resize() from the public API #367

dankamongmen opened this issue Feb 21, 2020 · 7 comments
Assignees
Labels
documentation Improvements or additions to documentation question/research Further information is requested
Milestone

Comments

@dankamongmen
Copy link
Owner

I don't think the user should ever need to call notcurses_resize() themselves, and it can be hidden. We call it in notcurses_render() at the end of the rendering step. Why don't we just call it before the rendering step, instead?

Am I missing something?

If we do hide it, be sure to remove it from all the docs, including the book.

@dankamongmen dankamongmen added documentation Improvements or additions to documentation question/research Further information is requested labels Feb 21, 2020
@dankamongmen dankamongmen self-assigned this Feb 21, 2020
@dankamongmen dankamongmen added this to the 1.3.0 milestone Feb 29, 2020
dankamongmen added a commit that referenced this issue Mar 1, 2020
@dankamongmen
Copy link
Owner Author

I've removed the indications in the documentation that this needs to be called manually for notcurses_render() to work properly, as I don't see how that's possibly the case.

I think we might want to move the screen geometry resample to the front of notcurses_render(), from the end, for the reasons that initially moved us to add that warning: if notcurses_render() runs thinking that lines are one length, and they are some other, shrinking the window will cause bad problems. Note, however, that this doesn't completely eliminate the problem, due to the inherent raciness of BSD signals...

@dankamongmen
Copy link
Owner Author

Removed the note from the book.

@dankamongmen dankamongmen changed the title Why does the user ever need to call notcurses_resize()? Remove notcurses_resize() from the public API Apr 8, 2020
@dankamongmen
Copy link
Owner Author

dankamongmen commented Apr 8, 2020

I have moved notcurses_resize() into internals, and removed its definition from all headers and docs. notcurses_refresh() now accepts int* restrict y, int* restrict x to return any new geometry, and calls notcurses_resize() internally. chunli, however, crashes sometimes now when I rapidly resize things:

==1872895== Invalid read of size 1
==1872895==    at 0x48635CC: notcurses_rasterize (render.c:884)
==1872895==    by 0x4863F6B: notcurses_refresh (render.c:1061)
==1872895==    by 0x10FDD4: chunli_draw (chunli.c:20)
==1872895==    by 0x1101D4: chunli_demo (chunli.c:61)
==1872895==    by 0x110C7D: ext_demos (demo.c:220)
==1872895==    by 0x1124FD: main (demo.c:532)
==1872895==  Address 0x7610330 is 16 bytes after a block of size 86,400 alloc'd
==1872895==    at 0x483677F: malloc (vg_replace_malloc.c:309)
==1872895==    by 0x4863EEA: notcurses_refresh (render.c:1053)
==1872895==    by 0x10FDD4: chunli_draw (chunli.c:20)
==1872895==    by 0x1101D4: chunli_demo (chunli.c:61)
==1872895==    by 0x110C7D: ext_demos (demo.c:220)
==1872895==    by 0x1124FD: main (demo.c:532)
==1872895== 
==1872895== Invalid read of size 8
==1872895==    at 0x4860458: cell_double_wide_p (notcurses.h:1052)
==1872895==    by 0x4863600: notcurses_rasterize (render.c:889)
==1872895==    by 0x4863F6B: notcurses_refresh (render.c:1061)
==1872895==    by 0x10FDD4: chunli_draw (chunli.c:20)
==1872895==    by 0x1101D4: chunli_demo (chunli.c:61)
==1872895==    by 0x110C7D: ext_demos (demo.c:220)
==1872895==    by 0x1124FD: main (demo.c:532)
==1872895==  Address 0x7370068 is 8 bytes after a block of size 57,600 alloc'd
==1872895==    at 0x48366AF: malloc (vg_replace_malloc.c:308)
==1872895==    by 0x4838DE7: realloc (vg_replace_malloc.c:836)
==1872895==    by 0x48619A0: reshape_shadow_fb (render.c:110)
==1872895==    by 0x4862633: notcurses_render_internal (render.c:469)
==1872895==    by 0x4864031: notcurses_render (render.c:1079)
==1872895==    by 0x118619: demo_render (hud.c:457)
==1872895==    by 0x110087: chunli_draw (chunli.c:41)
==1872895==    by 0x1101D4: chunli_demo (chunli.c:61)
==1872895==    by 0x110C7D: ext_demos (demo.c:220)
==1872895==    by 0x1124FD: main (demo.c:532)
==1872895== 
==1872895== Invalid read of size 8
==1872895==    at 0x4860458: cell_double_wide_p (notcurses.h:1052)
==1872895==    by 0x4863C6E: notcurses_rasterize (render.c:1007)
==1872895==    by 0x4863F6B: notcurses_refresh (render.c:1061)
==1872895==    by 0x10FDD4: chunli_draw (chunli.c:20)
==1872895==    by 0x1101D4: chunli_demo (chunli.c:61)
==1872895==    by 0x110C7D: ext_demos (demo.c:220)
==1872895==    by 0x1124FD: main (demo.c:532)
==1872895==  Address 0x7370068 is 8 bytes after a block of size 57,600 alloc'd
==1872895==    at 0x48366AF: malloc (vg_replace_malloc.c:308)
==1872895==    by 0x4838DE7: realloc (vg_replace_malloc.c:836)
==1872895==    by 0x48619A0: reshape_shadow_fb (render.c:110)
==1872895==    by 0x4862633: notcurses_render_internal (render.c:469)
==1872895==    by 0x4864031: notcurses_render (render.c:1079)
==1872895==    by 0x118619: demo_render (hud.c:457)
==1872895==    by 0x110087: chunli_draw (chunli.c:41)
==1872895==    by 0x1101D4: chunli_demo (chunli.c:61)
==1872895==    by 0x110C7D: ext_demos (demo.c:220)
==1872895==    by 0x1124FD: main (demo.c:532)
==1872895== 
==1872895== 

@dankamongmen dankamongmen reopened this Apr 8, 2020
dankamongmen added a commit that referenced this issue Apr 8, 2020
* packaging: s/libtinfo/Terminfo/g
* rust: add stddim_yx()
* rust: check for valid init in unit tests
* rust: serialize up tests
* constify notcurses_term_dim_yx()
* rust: add dim wrappers
* remove notcurses_resize() from public API #367
* call notcurses_resize() from notcurses_refresh() #367
@dankamongmen
Copy link
Owner Author

dankamongmen commented Apr 8, 2020

The reason for the crash is summarized in this existing comment near the top of notcurses_rasterize():

  // FIXME need to track outer{x,y} (position on screen) and inner{x,y}
  // (position within lastframe/rvec, which is lfdimx-sized)
  for(y = nc->stdscr->absy ; y < nc->stdscr->leny + nc->stdscr->absy ; ++y){

by calling notcurses_resize() at the beginning of notcurses_refresh(), we're changing leny and lenx, and they no longer match lfdimy and lfdimx. I think we would have seen the same behavior before, if someone called notcurses_resize() (following an actual resize event) and then called notcurses_refresh(). this didn't happen with notcurses_resize() followed by notcurses_render() because the latter rerenders first, bringing lfdimx up to sync with lenx etc, before calling notcurses_rasterize().

@dankamongmen
Copy link
Owner Author

I think ncplane_resize_internal() just needs to be extending and zeroing out lastframe as it does stdplane, and we'll be good. In fact...I think we could then get rid of lfdimy/lfdimx, since every resize (update of lenx/leny) would be followed by a render...no, nvm, but what we can do is chain resize_shadowfb() to the end of notcurses_resize(), so the stdscr and lastframe get updated at the same time, with the same effect. Yes!

@dankamongmen
Copy link
Owner Author

Got it! w00t!

@dankamongmen
Copy link
Owner Author

It's gone, and we're good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation question/research Further information is requested
Projects
None yet
Development

No branches or pull requests

1 participant