Skip to content

Correct the behavior of the Calendar component [APPS-02GT] #645

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

Merged
merged 4 commits into from
Nov 13, 2024

Conversation

sebai-dhia
Copy link
Contributor

Synchronize the currentField property with activeField in the LocalDate case to ensure normal behavior when making changes in a field, as an assertion may occur if this condition is not met. This corrects the abnormal behavior, initially caused by this assertion, where date changes made by the Vaadin pop-up were not saved.

@sebai-dhia sebai-dhia requested a review from mgrati November 8, 2024 16:23
@sebai-dhia sebai-dhia changed the title Synchronize currentFeild proprety with activeField in LocalDate case [APPS-02GT] Correct the behavior of the Calendar component [APPS-02GT] Nov 8, 2024
Copy link
Member

@mgrati mgrati left a comment

Choose a reason for hiding this comment

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

Please fix the review comments below.

build.gradle.kts Outdated
@@ -82,7 +82,7 @@ allprojects {
pom {
configureMavenCentralPom(project)
}
signPublication(project)
// signPublication(project)
Copy link
Member

Choose a reason for hiding this comment

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

This change is to not be commited

@@ -2011,7 +2011,7 @@ abstract class VField protected constructor(width: Int, height: Int) : VConstant
* Checks that field value exists in list
*/
@Suppress("UNCHECKED_CAST")
/*internal*/ fun selectFromList(gotoNextField: Boolean) {
/*internal*/ fun selectFromList(gotoNextField: Boolean) {
Copy link
Member

Choose a reason for hiding this comment

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

Fix this indentation

Comment on lines 2272 to 2275
block!!.getFieldPos(this),
label,
lab ?: name,
toolTip)
Copy link
Member

Choose a reason for hiding this comment

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

Fix indentation

Comment on lines 2330 to 2333
modeDesc,
getTypeName(),
getTypeInformation(),
names)
modeDesc,
getTypeName(),
getTypeInformation(),
names)
Copy link
Member

Choose a reason for hiding this comment

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

Fix this indentation

Comment on lines 76 to 78
if (getModel().getDataType() == LocalDate::class){
getModel().block!!.activeField = getModel()
}
Copy link
Member

Choose a reason for hiding this comment

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

We should generalise this. Since the problem is with field focus, do this instead :
if (!getModel().hasFocus()) {
getModel().block!!.gotoField(getModel())
}

Also, should we add the same check to DGridTextEditorField ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using this treatment inside the old one returns the same results. As for whether we should add the same check to DGridTextEditorField, we don’t need that because grid fields are not editable in Galite. However, if the behavior changes, it may be necessary.

Comment on lines 118 to 125
noEcho,
scanner,
noEdit,
align,
model.hasAutofill(),
this)
noEcho,
scanner,
noEdit,
align,
model.hasAutofill(),
this)
Copy link
Member

Choose a reason for hiding this comment

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

Fix this code indentation

Comment on lines 277 to 283
override fun toGui(modelTxt: String?): String? {
override fun toModel(modelTxt: String?): String? {
return modelTxt
}

override fun toModel(guiTxt: String?): String? {
override fun toGui(guiTxt: String?): String? {
Copy link
Member

Choose a reason for hiding this comment

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

I don't see the point of these changes ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made those changes just to make the function names more meaningful and aligns them with their respective purposes

Copy link
Member

Choose a reason for hiding this comment

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

I think there is a misunderstanding :
ToGui function aims to transform a model text into a text to be interpreted by the GUI. In this case, the GUI text is the same as the model text this is why we return the model text value.
The same logic applies to the function toModel.
You should restore these functions to their previous states.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, okay, I'll undo the change.

@sebai-dhia
Copy link
Contributor Author

sebai-dhia commented Nov 13, 2024

We corrected the indentation made by accident and made the issue more general by changing:

	if (getModel().getDataType() == LocalDate::class) {
	    getModel().block!!.activeField = getModel()
	}
to:
	if (!getModel().hasFocus()) {
	    getModel().block!!.gotoField(getModel())
               }
(Tests return the same results. So, we'll go with the general solution.)

As for whether we should add the same check to DGridTextEditorField, we don’t need that because grid
fields are not editable in Galite. However, if the behavior changes in the future, it might become necessary.

…in valueChanged of DGridTextEditorField [APPS-02GT]
@sebai-dhia
Copy link
Contributor Author

I misunderstood DGridTextEditorField, I thought it was a field in dynamic reports. To ensure the same functionality, we'll apply the same logic of the valueChanged function in DTextField as in valueChanged of DGridTextEditorField.

@mgrati mgrati merged commit f1fff68 into master Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants