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

Returning the focus to the original receiver #8649

Merged
merged 4 commits into from
Dec 7, 2023

Conversation

N-thony
Copy link
Collaborator

@N-thony N-thony commented Nov 13, 2023

Fixes #8256
@rdstern I have tested this with some dialogues in the Describe menu, have a look

Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

@N-thony it seems to work fine on some dialogs. Others resturn and nothing is in turquoise. Here is an example:

image

The stem and leaf is another like that:

image

@N-thony
Copy link
Collaborator Author

N-thony commented Nov 15, 2023

@rdstern have a look now, I noticed that some dialogues might have the same issue of tab orders that I have fixed in the Describe Two Variables. We should always make sure to have the correct order of the controls when improving any dialogue, I will mention that to the team because it is very important after looking at the code now.

Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

Looks fine. Perhaps @Patowhiz can confirm

@@ -63,6 +63,12 @@ Public Class ucrSelector
'always load selector contents on load event because contents may have been changed at R level
'and the control needs to refresh the data frame names.
LoadList()
'always return the focus to the first Receiver when re-opening the dialogue.
If lstOrderedReceivers.Count > 0 Then
Dim lstVisibleReceivers = lstOrderedReceivers.Where(Function(ctrl) ctrl.Visible).ToList()
Copy link
Contributor

Choose a reason for hiding this comment

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

@N-thony could you specify the type of lstVisibleReceivers explicitly. This has many merits relating to long term code maintenance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lstVisibleReceivers contains list of visible receivers especially in cases where we have different options in the dialogue.
This will allow to find the first receiver and set the focus on it. I noticed we are not paying attention to the tab orders in the dialogue, it has to be fixed, I have mentioned that ot the team.

Copy link
Contributor

Choose a reason for hiding this comment

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

@N-thony I understand what the predicate is doing in the where clause.
Could you then specify the type explicitly, for example Dim lstVisibleReceivers As List(Of Control). I understand that VB.Net allows dynamic types, but to be consistent with the code base and also for self documenting code, I strongly recommend static typing in this case.

@Patowhiz
Copy link
Contributor

Patowhiz commented Dec 5, 2023

@rdstern I hope you have tested this with dialogs that have multiple features. I'm particularly interested with dialogs that have multiple features yet use different receivers. The sequence of reload may cause regression in the changes introduced here.

@N-thony
Copy link
Collaborator Author

N-thony commented Dec 6, 2023

@rdstern I hope you have tested this with dialogs that have multiple features. I'm particularly interested with dialogs that have multiple features yet use different receivers. The sequence of reload may cause regression in the changes introduced here.

@rdstern you can test this esp to dialogue with different receivers, and let me know a possible issue or regression

rdstern
rdstern previously approved these changes Dec 7, 2023
Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

@Patowhiz I have checked again. I really like this improvement and did check with many dialogs. I can't see any problems. My nervousness is that you know of some problems and are really just trying to teach me the lesson that I'm not checking carefully enough?
If that's not the case, then please continue with your checks and then merge!

@Patowhiz
Copy link
Contributor

Patowhiz commented Dec 7, 2023

@rdstern thanks for your response. I'm glad you were able to check again on the dialogs that I was concerned about.
I'm still not sure whether this change may affect performance of dialog reloads or not when it comes to wide data sets.
@N-thony thanks for the change, would be useful to test this with dialogs that have different receivers that are visible in different options/features when using wide data sets. Nevertheless, I'm merging this.
@rdstern keep testing this feature in other PR's as well, I'll do the same.

@Patowhiz Patowhiz merged commit 5439433 into IDEMSInternational:master Dec 7, 2023
2 checks passed
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.

Returning the focus to the original location?
3 participants