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

UI - Input Component #203

Closed
4 of 5 tasks
akinsho opened this issue Jan 11, 2019 · 18 comments · Fixed by #644
Closed
4 of 5 tasks

UI - Input Component #203

akinsho opened this issue Jan 11, 2019 · 18 comments · Fixed by #644
Labels
A-components Area: Consumer-exposed components providing some convenience over the primitives

Comments

@akinsho
Copy link
Member

akinsho commented Jan 11, 2019

I noticed there wasn't an issue to track against adding an Input component to revery (I started having a look into this), not sure if we have all the pieces we need in place for at least a minimal implementation.

If there are any specific behaviors to keep in mind or anyone has any suggestions it would be nice to collect them here

Outstanding work

@bryphe bryphe added the A-components Area: Consumer-exposed components providing some convenience over the primitives label Jan 23, 2019
@akinsho
Copy link
Member Author

akinsho commented Jan 23, 2019

This is partially solved by #205 although outstanding issues include being able to move the cursor with the keyboard or on click to edit the input

@akinsho
Copy link
Member Author

akinsho commented Jan 25, 2019

@bryphe an interim solution for moving with the arrow keys (or a configurable set 😉 ) would be to draw the text around the cursor a bit like the commandline in oni but really to get parity with a browser input there'd need to be some way to figure what was clicked say we had "apple" and a user clicked the "a" any ideas how we might be able to get that information

I can't think of a way atm I know we probably get the xy coords but wondering if there might be a way to map that to what element/character is rendered there

@OhadRau
Copy link
Collaborator

OhadRau commented Jan 25, 2019

Looks like we can lump this in with the text wrapping maybe? It'll require us to have some coords<->character functions so we could knock out both at the same time. It's also a bunch of work though 😬

@schinns
Copy link
Contributor

schinns commented Feb 6, 2019

@Akin909 I am looking at the Input component and example and trying to understand onChange prop. was hoping the value labeled argument here would be similar to the event.target.value in the web js api. But when I tried to log first or second from state with print_endline or tried rendering as Text node— I got nothing. If the onChange method is not supposed to return the text that is being typed in the input, then what should the expected output be?

@faisalil
Copy link
Contributor

faisalil commented Feb 6, 2019

@schinns , I have a work around that is coming as part of a change I am sending in few seconds. Issue was discussed in the discord channel and here is what is happening: #305

@akinsho
Copy link
Member Author

akinsho commented Feb 6, 2019

@schinns as @faisalil noted (I should have noted it here so people didn't have to go exploring) there's a bug with state in the event handlers so I disconnected those as I was experimenting with trying to create controlled components but with the current bug was seeing unusual results.

Although the idea is that value should return something similar to event.target.value. There are many ongoing issues with the Input that need work which I'm trying to track in here I'll update the issue description with a list of all the known things that need work

@faisalil
Copy link
Contributor

faisalil commented Feb 9, 2019

Based on what @bryphe mentioned, fixing #290 will help fix the issue of multiple Inputs. I am planing to take a stab at #290. I am also planning to bring the API of Input closer to react-native TextInput.
@Akin909, do you have any other thoughts?

@akinsho
Copy link
Member Author

akinsho commented Feb 10, 2019

@faisalil that sounds great the input could do with some love. Another big issues is editing text in the input I wouldn't look at that as part of what you're working on as I think it'll be a big job basically we need to figure out a way that when you click on part of the input we can place a cursor where you clicked so you can start editing from there.

We'd need some way to be able to figure out either the character/index under the cursor. Otherwise with just position values you'd be in the wonky place of having to place the text using x,y values which would be weird, I was thinking it would be better if, if you clicked on a text field we had some way of knowing what part of the word you clicked, it will need some figuring out but that's definitely one big thing the input currently lacks

@saitonakamura
Copy link
Contributor

saitonakamura commented Feb 14, 2019

to figure out a way that when you click on part of the input we can place a cursor where you clicked so you can start editing from there.

I was doing some research (well, just thinking about this) on this while I was working on text wrapping. Naive approach for me would be to create a map during glyph rendering that looks like this

// string: ab cd
// ab
// cd
{
  lines: [{
      beginY: 0.,
      endY: 14., // calculated line height essentially
      symbols: [{
        beginX: 0.,
        endX: 1.4,
        offsetInString: 0, // a
      }, {
        beginX: 1.4,
        endX: 2.3,
        offsetInString: 1, // b
      }],
    }, {
      beginY: 14.,
      endY: 28.,
      symbols: [{
        beginX: 0.,
        endX: 1.5,
        offsetInString: 2, // c
      }, {
        beginX: 1.5,
        endX: 2.4,
        offsetInString: 3, // d
      }],
  }]
}

And use binary search (or just iteration search) to find to which letter click belongs

@akinsho
Copy link
Member Author

akinsho commented Feb 14, 2019

@saitonakamura that sounds like a better start than I've come up with 👍, question though re performance sounds like this map could sometimes be humungous if it occurs during glyph rendering and the string was huge. Wondering if this would result in a big memory/performance hit cc @bryphe or @OhadRau

@akinsho
Copy link
Member Author

akinsho commented Feb 14, 2019

the iteration would only happen on click so think that bit would be fine (🤷‍♂️) more concerning is storing some enormous list for all text elements throughout an application which using Oni2 as an example could be quite a lot.

A similar problem I've seen storing lists of very large sizes is how vscode handles syntax highlighting tokens in binary format using bit shifting so that its more memory efficient

@saitonakamura
Copy link
Contributor

saitonakamura commented Feb 14, 2019

storing some enormous list for all text elements throughout an application which using Oni2 as an example could be quite a lot.

I would say such application as oni should use virtualization technique to render only visible scope, disregardless of how we're gonna map the click to text position. In fact, I think it should be pretty straightforward given the constant line height, but I'm not, by any means, an expert in this field. If we're gonna use virtualization though, it'll drastically help to reduce the memory footprint of such mapping, because this map will be only for visible text

@akinsho
Copy link
Member Author

akinsho commented Feb 14, 2019

One more thing to note is that at the glyph rendering bit we'd have to make sure that the list was being updated rather than regenerated each time (maybe that goes without saying)

@saitonakamura
Copy link
Contributor

It's a good point, I haven't thought about it. Though I'm wondering if updating would be better in all cases. At a first glance I see five ways of triggering text render: Editing text, Pasting/Cutting text, Scrolling, Resizing window/panel and Changing font size/zooming

  1. Editing text would definitely benefit from updating, because we would be working with one line most of the time, hence we don't need to recalculate the others (watch out for multiple cursors though)
  2. Pasting/Cutting text is a tricky beast in that terms, cause it would be different from pasting a single char to cutting the whole visible piece (in which case updating could be even slower)
  3. Scrolling can really benefit from that as most of the scrolling happen in one direction, horizontal mostly (which means we would have to recalculate only y coords, plus add new lines and drop the others (caching x coords for line potentially)). Would be a different situation with the faster scrolling (throttle the update maybe?)
  4. Resizing window/panel - would benefit from it if we pin the left top corner of text view (so the resizing would only affect the visible part, but not the scrolling)
  5. Changing font size/zooming - this would completely changing everything. We could try to update the coords by multiplying them by something, though I'd argue that it's a common operation

@saitonakamura
Copy link
Contributor

saitonakamura commented Feb 16, 2019

Not sure if applicable, but worth looking at
https://en.wikipedia.org/wiki/Quadtre
https://github.com/mourner/rbush
https://en.wikipedia.org/wiki/Spatial_database#Spatial_index

@glennsl
Copy link
Member

glennsl commented Oct 27, 2019

A couple of issues with the current Input component:

  • The cursor doesn't seem able to keep up if you start mashing keys, like you do when you're testing something and just want some text.
  • It doesn't seem possible dynamically size the input, like using flexGrow(1), because the default width will override it and it doesn't seem possible to override that with something like width(auto) either,

@Et7f3
Copy link
Member

Et7f3 commented Nov 3, 2019

Controlled Inputs - relates to

Shouldn't be crossed #406 ? @Akin909

@glennsl
Copy link
Member

glennsl commented Nov 15, 2019

I've checked "Controlled inputs" since it was implemented in #406. I've also refined it in #644 to allow control over cursor position.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-components Area: Consumer-exposed components providing some convenience over the primitives
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants