Advanced text input options #25
Conversation
Signed-off-by: swan.seo <[email protected]>
Signed-off-by: swan.seo <[email protected]>
Swanseo0
force-pushed
the
advanced
branch
2 times, most recently
from
cfd1f69
to
b7946b2
Compare
Signed-off-by: swan.seo <[email protected]>
|
In order to compile NUI after this patch, need to use |
| Dali::Dimension::HEIGHT); | ||
| label.SetProperty(Dali::Toolkit::TextLabel::Property::TEXT_COLOR, | ||
| Dali::Color::WHITE_SMOKE); | ||
| label.SetProperty(Dali::Toolkit::TextLabel::Property::POINT_SIZE, 7.0f); |
There was a problem hiding this comment.
Q. Are there any default values for these properties? It'd be nice if we could avoid using magic numbers.
There was a problem hiding this comment.
The default text color is black and the point size is about 15.
The background color of the pop-up window is dark blue, so I can't see the letters well, so I set it to white smoke.
The font size is set to an appropriate size because the default value is large, but we will modify it so that we do not use the magic number.
However, if we do not change the background color of popup, white smoke will be better than the basic color(black).
There was a problem hiding this comment.
Does "white background" and "7.0f font size" look good for both tv and common (rpi) profiles?
+) NUI popup may differ from Dali popup. It might be better to set it equal to that value.
https://docs.tizen.org/application/dotnet/guides/user-interface/nui/nui-components/Popup/
|
|
||
| #include <dali-toolkit/devel-api/controls/popup/popup.h> | ||
|
|
||
| #include <functional> |
There was a problem hiding this comment.
Missing includes?
#include <dali/public-api/dali-core.h>
#include <string>Please consult me or @bbrto21 on how to organize includes in the implementation file (nui_autofill_popup.cc) in a consistent way.
| int ret = autofill_connect( | ||
| autofill_, | ||
| [](autofill_h autofill, autofill_connection_status_e status, | ||
| void* user_data) {}, |
There was a problem hiding this comment.
Is this deliberately no-op? No need to check the current connection status?
There was a problem hiding this comment.
connected_ have been added to check connection status.
There was a problem hiding this comment.
is_connected_ is usually better than just connected_.
There was a problem hiding this comment.
You shouldn't need the extra SetConnected method. You can directly access private member fields via user_data.
Also you shouldn't assume the status is always AUTOFILL_CONNECTION_STATUS_CONNECTED.
There was a problem hiding this comment.
The if (!connected_) check will fail if it comes immediately after Initailize() and autofill_connect is not a synchronous call.
Related to the comment #25 (comment).
There was a problem hiding this comment.
is_connected_is usually better than justconnected_.
Modified to use is_connected_.
You shouldn't need the extra
SetConnectedmethod. You can directly access private member fields viauser_data.
Can I access the private region by casting user_data to TizenAutofill?
Also you shouldn't assume the
statusis alwaysAUTOFILL_CONNECTION_STATUS_CONNECTED.
Changed to call SetConnected according to state.
The if (!connected_) check will fail if it comes immediately after Initailize() and autofill_connect is not a synchronous call.
Autofill will not work until connected. So we need to make sure it's connected.
There was a problem hiding this comment.
Can I access the private region by casting
user_datatoTizenAutofill?
In class methods, yes, you can. Also please do not rename user_data to data unless it is appropriate. We often use self as the name of the casted pointer.
There was a problem hiding this comment.
Thank you for advice.
The data was changed to user_data again and use self as the name of the casted pointer.
| FT_LOG(Error) << "connect_autofill_daemon error"; | ||
| } | ||
|
|
||
| autofill_fill_response_set_received_cb( |
There was a problem hiding this comment.
No need to check the return value in this case?
There was a problem hiding this comment.
Changed it to check the return value of this case.
Additionally, i have added a intialized_ variable to save whether the initialization process has all been successfully performed.
There was a problem hiding this comment.
You should not immediately return after Initailize.
Also Initailize→Initialize.
There was a problem hiding this comment.
You should not immediately
returnafterInitailize.
The reason for immediate return after Initialize is that connect is asynchronous.
If proceed with the autofill request immediately after initialize, it will fail because it is not connected.
Also
Initailize→Initialize.
Modified.
There was a problem hiding this comment.
That's the point of my comment above. The RequestAutofill method has indeterminate behavior depending on whether the TizenAutofill is initialized or not. If it's not initialized (although it's unlikely in normal situations), the method will simply call Initialize and exit. If it is already initialized, the rest of the method body will be executed.
There was a problem hiding this comment.
Sorry for the late reply.
The reason you said "You should not immediate return after initiation" is because the connection is asynchronous, and we need to wait for the connection instead of return right away? Or should we not call the Initialize function at RequestAutofill function?
There was a problem hiding this comment.
The reason you said "You should not immediate return after initiation" is because the connection is asynchronous, and we need to wait for the connection instead of
returnright away?
Kind of yes.
Or should we not call the
Initializefunction atRequestAutofillfunction?
I don't know. That's what you have to think in the design phase.
Swanseo0
force-pushed
the
advanced
branch
2 times, most recently
from
41b4629
to
695b8b6
Compare
| Dali::Dimension::HEIGHT); | ||
| label.SetProperty(Dali::Toolkit::TextLabel::Property::TEXT_COLOR, | ||
| Dali::Color::WHITE_SMOKE); | ||
| label.SetProperty(Dali::Toolkit::TextLabel::Property::POINT_SIZE, 7.0f); |
There was a problem hiding this comment.
Does "white background" and "7.0f font size" look good for both tv and common (rpi) profiles?
+) NUI popup may differ from Dali popup. It might be better to set it equal to that value.
https://docs.tizen.org/application/dotnet/guides/user-interface/nui/nui-components/Popup/
|
We can test using this autofill sample.
Four items with AUTOFILL_HINT_ID have been added from |
| void TizenAutofill::RequestAutofill(std::vector<std::string> hints, | ||
| std::string id) { | ||
| if (!initailzed_) { | ||
| Initailize(); |
There was a problem hiding this comment.
Initialize has already been requested in the constructor. Do you need to initialize again in case of initialization failure?
There was a problem hiding this comment.
Initialize failed case is when autofill_create or autofill_connet failed.
In this case, TizenAutofill is not expected to work at all, so I think it needs to be reinitialized.
There was a problem hiding this comment.
Isn't Initialize likely to fail again if it failed before?
There was a problem hiding this comment.
When it fails and tries again, it seems likely to fail again. So I changed it so that it doesn't try Initialize again.
| void* data) { | ||
| int count = 0; | ||
| autofill_fill_response_get_group_count(fill_response, &count); | ||
| autofill_fill_response_foreach_group( |
There was a problem hiding this comment.
You could make each nested callback into a private static method (or a named lambda expression) to reduce the level of indentation.
very long nested anonymous functions can make code harder to understand.
Otherwise, you could wrap some of the C functions into individual methods (in the sense of OOP) but I'm not sure if it's appropriate. You may redesign this entire class if you feel it's appropriate.
Also is it okay to just ignore the return values (ret) of these functions?
There was a problem hiding this comment.
You could make each nested callback into a private static method (or a named lambda expression) to reduce the level of indentation.
Reduced the level of identation.
Otherwise, you could wrap some of the C functions into individual methods (in the sense of OOP) but I'm not sure if it's appropriate. You may redesign this entire class if you feel it's appropriate.
C functions with specific purposes were made into static functions.
Also is it okay to just ignore the return values (
ret) of these functions?
Added conditions for functions that need to be checked for ret.
I think other functions doesn't need to be checked, but is there anything I missed?
There was a problem hiding this comment.
Reduced the level of identation.
You may prefer static member methods over static functions, to allow accessing non-public members within callbacks.
I think other functions doesn't need to be checked,
Is it because you think you can simply "ignore" if they fail? Well, that's quite uncommon.
There was a problem hiding this comment.
The function that does not check the return value based on the current code is if the return value is AUTOFILL_ERROR_NONE or AUTOFILL_ERROR_INVALID_PARAMETER.
In the case of AUTOFILL_ERROR_INVALID_PARAMETER, I thought it could be ignored because the appropriate function was called with the appropriate parameters. Do I need to check this case?
There was a problem hiding this comment.
It depends. If you're sure the functions won't raise any errors or the errors can be ignored, error checking might not be necessary.
| void TizenAutofill::RequestAutofill(std::vector<std::string> hints, | ||
| std::string id) { |
There was a problem hiding this comment.
Always prefer const & types if the arguments are immutable.
There was a problem hiding this comment.
You don't have to apply this to pointer types like autofill_view_info_h.
Also you missed an & in line 136.
There was a problem hiding this comment.
I modified it. Thank you
Swanseo0
force-pushed
the
advanced
branch
3 times, most recently
from
12bb752
to
932e62c
Compare
| Ecore_IMF_Input_Panel_Return_Key_Type return_key_type = | ||
| ECORE_IMF_INPUT_PANEL_RETURN_KEY_TYPE_DEFAULT; | ||
|
|
||
| // Not support : none, previous, continueAction, route, emergencycall, newline |
There was a problem hiding this comment.
In English, a colon should not be preceded by a space.
| // Not support : none, previous, continueAction, route, emergencycall, newline | |
| // Not supported: none, previous, continueAction, route, emergencyCall, newline |
| void TizenInputMethodContext::SetEnableSuggestions(bool enable) { | ||
| FT_ASSERT(imf_context_); | ||
| ecore_imf_context_prediction_allow_set(imf_context_, | ||
| enable ? EINA_TRUE : EINA_FALSE); |
There was a problem hiding this comment.
We may need to organize redundant function calls in SetContextOptions and SetInputPanelOptions.
There was a problem hiding this comment.
SetContextOptions was removed. and redundant function calls in SetInputPanelOptions function were removed and renamed to SetInputPanelOption.
| void SetOnPopupAutofillContext(OnPopupAutofillContext callback) { | ||
| on_popup_autofill_context_ = callback; | ||
| } | ||
|
|
||
| void PopupAutofillItems() { | ||
| if (on_popup_autofill_context_) { | ||
| on_popup_autofill_context_(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Does this code have anything to do with TizenInputMethodContext? I'm not sure why this callback is registered in this class. In fact the names are a bit confusing (OnPopup, SetOnPopup, SetOnPopupAutofillContext, PopupAutofillItems, ...) and I had a hard time understanding the code.
There was a problem hiding this comment.
Sorry for the confusion. This functions were deleted because it was confirmed that the operation was operating normally even without this callback registration part.
| } | ||
|
|
||
| void TizenAutofill::RegisterItem(const std::string& view_id, | ||
| const AutofillItem& item) { |
There was a problem hiding this comment.
It is not currently in use, but it is a function that was implemented because I thought it would be needed someday. Was it an unnecessary implementation?
There was a problem hiding this comment.
RegisterItem function was deleted.
| struct AutofillItem { | ||
| autofill_hint_e hint_; | ||
| bool sensitive_data_; | ||
| std::string label_; | ||
| std::string id_; | ||
| std::string value_; | ||
| }; |
There was a problem hiding this comment.
Struct members should not be named like class members.
https://google.github.io/styleguide/cppguide.html#Variable_Names
Also, are the values other than value_ ever used?
There was a problem hiding this comment.
Mainly used are hint, label, and value.
sensitive_data and id are variable that are not used in RequestAutoflll. I think it will only be used in RegisterItem.
There was a problem hiding this comment.
Deleted obsolete hint_, sensitive_data_, and id_ while deleting the RegisterItem function.
| } | ||
| } | ||
| // TODO(Swanseo0): Change ctxpopup's position to focused input field. | ||
| evas_object_move(ctxpopup_, 0, 0); |
There was a problem hiding this comment.
Isn't the popup supposed to appear just above the keyboard?
There was a problem hiding this comment.
We can set it to appear just above the input panel. Should I temporarily set it to appear just above the input panel?
There was a problem hiding this comment.
I changed it to be located above the input window as much as possible. view nui could be placed completely above, but in the case of window elementary and view elementary, it can be misplaced depending on the anchor point.
This link is broken. |
Support text input options
enableSuggestionsinputActionautofillConfigurationTo support
TextInput.requestAutofill, the following features have been implemented.tizen autofillnui,elementaytype's popup for autofill (ecore wl2 type doesn't yet provide)Related to issue #6