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

Upgraded input #241

Closed
wants to merge 15 commits into from
Closed

Upgraded input #241

wants to merge 15 commits into from

Conversation

l3aShn9l
Copy link

No description provided.

Copy link
Owner

@Insality Insality left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello! Thanks for your work and your PR.

I'm noticed that here a lot of changes what is not relative to the issue in PR, but anyway I leave some comments about this.

That's about Input:
The cursor really should be and seems it can work good, but seems to better implement all additional logic right inside input component without add new component

You can add parameter in input style table to show "is cursor enabled' and add your logic inside input component

Also if your do the PR, please do it in develop branch, since the master contains only released versions. It's help me and everyone to easily check changes between versions

If you want to finish you work, the PR should be minimal changes for only Input component and cursor works.

Please write what do you think about all comments and this one?

Have a good day!

if _instances[i].url == url then
_instances[i]._priority = value
else
if freeze_or_unfreeze == "freeze" then
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All code about freeze/unfreeze is redundant due the BaseComponent:set_input_enabled function.
It's works on all components and don't pass the user input if it's disabled

Copy link
Author

@l3aShn9l l3aShn9l Aug 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All code about freeze/unfreeze is redundant due the BaseComponent:set_input_enabled function. It's works on all components and don't pass the user input if it's disabled

This work is result of my student practice. And our tutor ask me to make switching between components in one click and reset input if there was interaction with another component , because it will be more comfortable for user. Switching between components which belong to one game object can be make with BaseComponent:set_input_priority, but it does not work for components which belong to different game objects. As I understand I can't set priority to game objects directly. I found out that call of msg.post(, "acquire_input_focus") will make game object the most prioritized, that gives ability to sort instances. But there was problem: if i select input component, it's priority and priority of instance must be lower then priority of another components and instances because I need switching in one click, if priority will be higher another will not be selected. Selected input component will "eat" our click. Low priority cause problem which I solve with freeze/unfreeze. Components with keyboard input (buttons, back handler) will have higher priority than selected input, and they will read keyboard input, which they should not. Thats why I add ability to disable keyboard input. If I disable they will not be clickable. May be there could be another solution. May be global flag about selection of input component, but it also strange. May be You can recommend how block only keyboard input.

Здесь то же самое только на русском, так как я мог не совсем точно и корректно описать все:
Вся эта работа результат студенческой практики. Организация и куратор попросили ко всему прочему реализовать переключение и сброс Input в один клик, аргументируя тем, что переключение минимум через два неудобно для пользователя. И если сделать переключение между элементами внутри одного game object не является проблемой за счет BaseComponent:set_input_priority, то для того, чтобы это провернуть с элементами из разных, нужно регулировать приоритет instances и game objectes, единственное, что я смог придумать, это использовать msg.post(, "acquire_input_focus"), что делает game object наиболее приоритетным в плане ввода, как я понял. Для того, чтобы нажатия обрабатывались в один клик, приоритет выбранного input элемента должен быть ниже, чем остальные элементы, для элементов из разных game objects необходима чтобы instance имел меньший приоритет относительно остальных иначе могут происходить ситуации, что у нас выбрано 2+ input элемента или выбор в 2 клика, что меня попросили исправить. Но все это вызывает проблему, что при вводе в выбранный input, другие элементы (button, back handler) имея больший приоритет съедают ввод, для этого я и добавил freeze/unfreeze. Если Вы подскажите, как можно более лаконично и корректнее решить этот вопрос, то это будет отлично. Поскольку мне первое, что приходит в голову создать глобальный флаг о том, что у меня ввод в input, что так себе идея.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Давай на русском)

Там в input компоненте я начал добавлять для этой фичи - NO_CONSUME_INPUT_WHILE_SELECTED, но надо проверить что и как работает, т.к. помню что не все оч хорошо с ним

В любом случае, чтобы дальше с PR поработать, решение с фриз и прочими нужно убрать, а все изменения для input - сделать только в input. По идее самого кода так много не будет

Когда такой объем ПР и кажется, что изменения не оч связанны - сложней разбираться

Глобальный флаг - плохо, т.к. сложно контролируется и могут быть коллизии разные

Можешь посмотреть на NO_CONSUME_INPUT_WHILE_SELECTED и глянуть, похоже ли на это и может в ту сторону покопаем? что думаешь?

Ну и остальные комменты актуальные к ПР

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Я посмотрел NO_CONSUME_INPUT_WHILE_SELECTED, и понял, что то, что я хотел, можно было сделать парой строчек.
В итоге вышло, что NO_CONSUME_INPUT_WHILE_SELECTED можно и не включать, однако я не знаю, насколько найденное решение нормальное.
В Input.on_input, где мы обрабатываем actions я добавил:
if action_id == const.ACTION_TOUCH or action_id == const.ACTION_MULTITOUCH then
return false
end

Тем самым действия мышью не должны захватываться Input.

Но перед этим я включил NO_CONSUME_INPUT_WHILE_SELECTED и попытался добиться желаемого эффекта. При наивысшем приоритете, actions, обработка которых в условии имела часть "action.released", срабатывает ок, но если там что-то иное, как у backspace: action.pressed or action.repeated, то при наличии, например, backhandler, который срабатывает на backspace, происходит перехват backhandler'ом нажатой клавиши, то же самое, если на экране будут существовать кнопки, имеющие key_trigger на пробел (ну или клавишу символьную).
Попытка поиграться с if не дала особых результатов. Стало понятно, что нужно как-то сепарировать ввод мышки от клавиатуры. И самым простым показалось обрабатывать ACTION_TOUCH и т.п. и возвращать false.


Проверял я в начале на примере, который внутри библиотеки, но потом решил попробовать это же только создав другой. Там не использовалась collection factory, а просто создано 2 game object'a, каждый из которых со своим gui. И тут началась та же проблема: кнопка из другого go готова "съесть" любое нажатие backspace, enter, пробел, в зависимости от того, какой я key_trigger поставлю. А внутри go кнопка работала нормально. Вероятно в выполненной до этого работе я что-то поломал, т.к. у меня и в библиотечном примере back_handler съедал backspace, это, в принципе, и стало причиной добавления freeze/unfreeze. Но между go захват ввода все равно возникает.

Решение рабочее для примера, вроде как, найдено. Но в общем я еще подумаю, что можно придумать, чтобы между двумя независимыми go не было проблем. Может у Вас есть какие-то идеи почему это так и как теоретически это можно поправить?

if _instances[i]._deleted then
table.remove(_instances, i)
end
end

sort_instances()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we get druid instances, and we don't want to do extra work here.
Also we here resubscribe some callbacks, which is not good to

@@ -107,6 +157,7 @@ function M.new(context, style)

local new_instance = druid_instance(context, style)
table.insert(_instances, new_instance)
local instances = get_druid_instances()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inexplicit call of druid components sort, which seems not good
Also the instances is not used

@@ -291,8 +303,10 @@ function Input.select(self)
gui.reset_keyboard()
self.marked_value = ""
if not self.is_selected then
self:set_input_priority(const.PRIORITY_INPUT_MAX, true)
self.button:set_input_priority(const.PRIORITY_INPUT_MAX, true)
print("select")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of print functions is not removed in this PR

print("select")
self.druid:set_priority(0, "freeze")
self:set_input_priority(0, true)
self.button:set_input_priority(100, true)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better in Druid code use priority constants from conts.PRIORITY_*

@@ -30,6 +30,14 @@ key_trigger {
input: KEY_G
action: "key_g"
}
key_trigger {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good! If we rework input component it can be here for everyone

gui.set_text(gui.get_node("upgraded_input_header_1"), "Something: " .. text)
end

function global_func(self, text)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Global funcs is bad in any situations, don't understand for which reason did you put it here?

@@ -249,6 +277,10 @@ function DruidInstance.initialize(self, context, style)
self._input_blacklist = nil
self._input_whitelist = nil

self.set_pr = Event()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also all of these events are redundant and things it's solved can be done via already existing Druid API

@@ -84,7 +84,7 @@ M["scroll"] = {
INERT_SPEED = 30, -- koef. of inert speed
EXTRA_STRETCH_SIZE = 100, -- extra size in pixels outside of scroll (stretch effect)
POINTS_DEADZONE = 20, -- Speed to check points of interests in no_inertion mode
WHEEL_SCROLL_SPEED = 0, -- Amount of pixels to scroll by one wheel event (0 to disable)
WHEEL_SCROLL_SPEED = 40, -- Amount of pixels to scroll by one wheel event (0 to disable)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't change default style here. It's also non documented and better to disable mouse scroll by default (it's still not perfect)

@@ -180,7 +192,8 @@ function Scroll.init(self, view_node, content_node)
self.drag.on_touch_end:subscribe(self._on_touch_end)

self.hover = self.druid:new_hover(view_node)
self.hover.on_mouse_hover:subscribe(self._on_mouse_hover)
self.hover.on_mouse_hover:subscribe(on_scroll_mouse_hover)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The on_mouse hover calls even it's hover or going away already
The on_mouse_away function is redundant

@Insality Insality added the enhancement New feature or request label Aug 20, 2023
@Insality Insality changed the base branch from master to develop August 22, 2023 21:15
@Insality
Copy link
Owner

Closed due another PR from your team about input field: #258

@Insality Insality closed this Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants