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

Wpf DataGrid in .NET7 takes away F3 and automatically sorts. - Breaking change. #7288

Open
RajeshAKumar opened this issue Nov 14, 2022 · 33 comments
Labels
Bug Product bug (most likely)
Milestone

Comments

@RajeshAKumar
Copy link

Our application uses Wpf DataGrid and was working fine till .NET 6.0
When upgraded to .NET 7, press F3 the application expectation is to take to Main window's Search box, but the grid takes over and does the sort of a column where F3 was pressed.

This breaks the functionality.

Is there a way to disable this?

@miloush
Copy link
Contributor

miloush commented Nov 14, 2022

/cc @merriemcgaw @rperez030 as expected we are now getting reports from customers whose application got broken by #6737 (PR #6873)

@RajeshAKumar as you can see in the PR this change cannot be opted out. Unfortunately the easiest way for you might be to handle the shortcut on PreviewKeyDown. Or, you can disable the sorting and handle that manually.

@RajeshAKumar
Copy link
Author

I understand the change, but why would you add such stuff that reworks the normal usage of F3. F3 is for search in windows platform, if you wanted to add something like this, you could have added a property to disable that behavior.

We also have tri-level multi-column sort on the grid implemented and of course we do not want a keyboard shortcut for it. We have a button and a UI for it.

How about you also apply CTRL + S to close the grid or print preview the grid? Will that make sense, when common use case on all windows applications for CTRL + S is to save?

Please tell me know can I trap this, but bubble it up the chain so main window's F3 will work correctly?

@miloush
Copy link
Contributor

miloush commented Nov 14, 2022

Would setting CanUserSort to false work for you?

Otherwise you might need to create your own class deriving from DataGridCell that blocks the shortcut getting to the base and your own class deriving from DataGridCellsPresenter that returns your cell class.

@RajeshAKumar
Copy link
Author

We also support user's ability to sort via clicking on column headers.
So no, we cannot set CanUserSort to false.

@RajeshAKumar
Copy link
Author

RajeshAKumar commented Nov 15, 2022

I was able to temporarily handle this manually.

        protected override void OnPreviewKeyDown(KeyEventArgs e)
        {
            if (e.Key == Key.F3)
            {
                GetSearchControl()?.Focus();
                e.Handled = true;
                return;
            }

            base.OnPreviewKeyDown(e);
        }

@RajeshAKumar
Copy link
Author

RajeshAKumar commented Nov 15, 2022

However this is not an acceptable solution, since instead of F3, I have to find the ApplicationCommands.Search's equivalent key. Not sure if this changes based on Locale, etc.

The right way in my opinion to add a behavior to generic control, Framework, libraries without breaking the normal out of box behavior is to add a property to opt-in. If the default behavior out of box is changed, at minimum have an property/ option to opt-out dynamically or via better yet via Xaml.

This feature is not thoroughly thought through and looks like slipped through the cracks when some 'smart developer' applied the change without understanding implications of it.
I am not sure how change was 'Reviewed' and 'Approved'.

@miloush
Copy link
Contributor

miloush commented Nov 15, 2022

Please refer to the discussion in #6737 how this got in despite it being a breaking change.

@RajeshAKumar
Copy link
Author

Yes I saw that first and could not open that issue so had to open a new issue.
I see that @merriemcgaw seems to have authorized the change (looks like after the fact) to match WinForms behavior.

I don't think she understands that these are 2 different systems and not much common and did not think of users having to painfully fix this on 1000s or even millions of apps rather than preventing that breaking change or forcing the dev to add a mechanism to switch it off.

@RajeshAKumar
Copy link
Author

RajeshAKumar commented Nov 15, 2022

I also see that @pchaurasia14 being an Sr. Eng manager for WPF, approved this.
Looks like Microsoft is no longer what it used to be with solid codebase, may be too many and too fast changes?

@RajeshAKumar
Copy link
Author

@miloush @shanselman @AArnott : I suggest add a higher priority bug fix before this becomes even more of widespread issue and more such bugs are reported and WPF users/ devs are frustrated with this out of box .NET7 upgrade experience.
So hopefully when .net 7.0.1 or 7.0.x comes out there could be a fix with an option to shut this off, ideally via a Xaml property.

@miloush
Copy link
Contributor

miloush commented Nov 15, 2022

While I agree with you that this change was unfortunate and I would support reverting it or providing an alternative solution, I don't think calling names will help the case.

@RajeshAKumar
Copy link
Author

RajeshAKumar commented Nov 15, 2022

Per my understanding of the WPF DataGrid design, the behaviors and functionality is part of the DataGrid.
The commanding interface, UI and triggers are out of scope and user supported external to grid.

@RajeshAKumar

This comment was marked as off-topic.

@RussKie
Copy link
Member

RussKie commented Nov 15, 2022

@RajeshAKumar I appreciate it you're upset, however, there is no need to personal attacks or name calling. This is against the code of conduct. There's also no need to tag random people who do not work on WPF codebase.

@merriemcgaw
Copy link
Member

I do agree that this isn't an ideal implementation, and it would have been nice to make this a configurable key. In servicing WPF team could probably introduce a runtimeconfig.json feature switch as we have done for other areas (see this proposal). The feature switch value could replace (with appropriate error handling) the hard coded F3 key and allow developers to configure what their DataGrid uses for a sort key.

@RajeshAKumar
Copy link
Author

@RajeshAKumar I appreciate it you're upset, however, there is no need to personal attacks or name calling. This is against the code of conduct. There's also no need to tag random people who do not work on WPF codebase.

Thank you. I edited the content.

@RajeshAKumar
Copy link
Author

I do agree that this isn't an ideal implementation, and it would have been nice to make this a configurable key. In servicing WPF team could probably introduce a runtimeconfig.json feature switch as we have done for other areas (see this proposal). The feature switch value could replace (with appropriate error handling) the hard coded F3 key and allow developers to configure what their DataGrid uses for a sort key.

Yes that might work. Alternatively a property that allows us to set at Resource Dictionary level or even App.Xaml/Generic.xaml resources level, so different sections of code base can opt-in or opt-out instead of a global runtime switch.
In our binaries there are multiple executables, so a global runtime switch may not be a sufficient in that use case.
Configurable prop like SortAcceleratorKey="Keys.F3" by default and could be set to "Keys.None" or something like that will allow nested resourcedict settings.

@merriemcgaw
Copy link
Member

@RajeshAKumar - Those could be great options for .NET 8, but for .NET 7 the WPF team is limited by not being able to add new public surface area during servicing of the release. But I think we could argue that adding an opt-out or even the ability to configure based on a configuration we aren't breaking that rule. I'll work with the WPF team and the servicing shiproom to make sure something is approved as soon as possible. We service monthly so I would expect something soon.

@pchaurasia14 / @dipeshmsft / @anjalisheel-wpf FYI

@RajeshAKumar
Copy link
Author

RajeshAKumar commented Nov 15, 2022

@RajeshAKumar - Those could be great options for .NET 8, but for .NET 7 the WPF team is limited by not being able to add new public surface area during servicing of the release. But I think we could argue that adding an opt-out or even the ability to configure based on a configuration we aren't breaking that rule. I'll work with the WPF team and the servicing shiproom to make sure something is approved as soon as possible. We service monthly so I would expect something soon.

@pchaurasia14 / @dipeshmsft / @anjalisheel-wpf FYI

@merriemcgaw Thank you for considering the fix. I appreciate that you are looking into possibly a fix by next SR.
I also hope that the Engg processes be improved to enable smoother product delivery.

@anjali-wpf anjali-wpf added Bug Product bug (most likely) and removed Untriaged Requires WPF team triage labels Nov 16, 2022
@merriemcgaw
Copy link
Member

@RajeshAKumar to set expectations, the next SR, due to holidays in the US is likely to be in February. @pchaurasia14 and I talked about it today and they are prioritizing the fix but as you can likely understand, we can't make a firm commitment on date because many other things may wind up impacting when something will ship 😄 . That said, this is prioritized to be addressed in an SR sooner rather than later for sure!

@RajeshAKumar
Copy link
Author

@RajeshAKumar to set expectations, the next SR, due to holidays in the US is likely to be in February. @pchaurasia14 and I talked about it today and they are prioritizing the fix but as you can likely understand, we can't make a firm commitment on date because many other things may wind up impacting when something will ship 😄 . That said, this is prioritized to be addressed in an SR sooner rather than later for sure!

Thank you for ensuring this will be in next SR. ~Feb is okay.

@RajeshAKumar
Copy link
Author

Any update on the release of this?

@pchaurasia14
Copy link
Contributor

The fix is already merged for .NET 7 servicing release.
Should be available in upcoming 7.0.3 release.

@RajeshAKumar
Copy link
Author

The fix is already merged for .NET 7 servicing release. Should be available in upcoming 7.0.3 release.

Thank you everyone for getting this out.

@RajeshAKumar
Copy link
Author

I tried this with 7.0.4 and the issue still exists.

@Kuldeep-MS
Copy link
Contributor

Kuldeep-MS commented Mar 27, 2023

@RajeshAKumar - I have retested the fix for .net7 and I am able to see it is working.
I am attaching the repro app and writing steps down below for testing which you can replicate in your project for enabling/disabling the feature.

  1. Unzip the repro app and launch it through VS.
  2. Add the following code to your .csproj file.
<Project>
....
      <ItemGroup>
		
          <!-- App Context Switch-->				
          <RuntimeHostConfigurationOption Include="System.Windows.Controls.DisableDataGridKeyboardSort" Value="true"/>
	
    </ItemGroup>
....
</Project>
  1. Modify the value {false||true} for enabling/disabling the functionality.

Let me know if any step is unclear or confusing.
grid.zip

@RajeshAKumar
Copy link
Author

@Kuldeep-MS I did not know of the App-Context switch.

However this is still a breaking change and out of box without adding the switch, F3 is mapped to column sort.
This is breaking change not just on WPF grid, but also from a Windows platform consitency.
Pretty much it is universal on Windows (OS and Apps both from Microsoft and others), that F3 maps to "Search".

We have multiple WPF apps using the grid, so this requires a code change on all of them to add a "switch".

Hence I think you guys got the Out of Box default wrong.
You could have a switch that maps a given key code to opt-in and by default leave it to blank, so it will behave the same way before this breaking change was introduced!

@RajeshAKumar
Copy link
Author

@pchaurasia14 @merriemcgaw : Any thoughts on how to prevent this breaking change out of the box?
Please see my comments above

@miloush
Copy link
Contributor

miloush commented Mar 28, 2023

Since .NET 8 is on long-term support unlike .NET 7, I do want to put out for consideration reverting this change and implementing it properly. Moreover, as far as I can tell, the original issue that initiated this change did not ask for a keyboard shortcut to sort a column, it asked for the ability to sort using keyboard (in fact it asked for it in a specific application). This could be equally solved by making the DataGrid header keyboard-focusable, like for example Windows Explorer can do in the details view. The same treatment should apply to GridView in ListViewBox.

@RajeshAKumar
Copy link
Author

RajeshAKumar commented Mar 29, 2023

This is a breaking change and does not help many use cases. It was put together in a hurry without understanding the impacts.
The big issue is the fix made also chooses a wrong default causing breaking change out of the box.
@miloush I do not think we should wait till v8, but expect a proper fix to this in v7.

This is very strange compared to a lot of new features and fixes I have enjoyed since .NET 1.1, but this is not thought and implemented right. Why rush for initial breakage and why rush again for the fix that came out in 7.03?
When you were coming out with solution for this issue, would it not help if you ask folks who had an issue with this or people who raised the bug if the solution will work for them or not?

All other devs/ users who have not migrated yet to .NET 7 and when they migrate will also discover that an accidental F3 will start sorting the column. This is far from being over.
Think through this.

@pchaurasia14
Copy link
Contributor

pchaurasia14 commented Mar 29, 2023

@RajeshAKumar -

I did not know of the App-Context switch.

We tried to communicate it as clearly as possible - see #7288 (comment) from Merrie and PR #7297 description

This is breaking change not just on WPF grid, but also from a Windows platform consitency.
This is a breaking change and does not help many use cases

You are correct, this is a technical breaking change - and we should have documented it in such way. Often innovation requires us to make such technical breaking changes. While it diverts from Windows platform, it aligns with WinForms - another .NET UI platform.

this requires a code change on all of them to add a "switch".

The opt-out does NOT require a code change. You can change only the CONFIG.

Hence I think you guys got the Out of Box default wrong.
The big issue is the fix made also chooses a wrong default causing breaking change out of the box.

This was a conscious decision. If we hide new features and innovations under opt-in switches, they will be hard to discover and majority of the users will not get the benefit. Therefore, we prefer to make new features on by default. Sometimes at the cost of small technical breaking changes. And as such, sometimes we can make wrong judgements and break things we should have not. In such cases, the important thing is to react in reasonable time. However, in this specific case, we do not yet believe we made a mistake.

It was put together in a hurry without understanding the impacts
... but this is not thought and implemented right ...

We did not add this feature in a hurry. It was part of .NET 7.0 release. As in all SW engineering, it doesn't mean all decisions are perfect (I wish they were, but I know they never are). However, in this case, we still think it was the right decision - note that it was also made and implemented by WinForms, which influenced a little bit our decision as well.

Why rush for initial breakage and why rush again for the fix that came out in 7.03?

We did NOT rush the 7.0.3 change.  We reacted to feedback from you and added a way for users to not be impacted by the technical breaking change. Opt-out was the best option. It was clearly communicated what we are going to do. I am surprised that now after 2 months the plan is being questioned.

When you were coming out with solution for this issue, would it not help if you ask folks who had an issue with this or people who raised the bug if the solution will work for them or not?

See the links above. We think we communicated it as much as we could.

All other devs/ users who have not migrated yet to .NET 7 and when they migrate will also discover that an accidental F3 will start sorting the column. This is far from being over.

This is a fair point. We have different opinion. The time will prove us right or wrong. Let's all observe how many people are impacted by the breaking change and for how many the opt-out switch is not a sufficient solution. We hope the number of users impacted will be small. So far, it seems you are the only person who noticed the breaking change. If we are wrong and more users will be impacted, we may reconsider our decision.

@pchaurasia14
Copy link
Contributor

@miloush - Let's start a new discussion about ideal solution for the feature pretending we never did anything in the space yet.

When we have agreement on the ideal solution, we can then come back and see how practically implement it (or its parts only) in current situation where we already made changes and added opt-out in servicing.

@merriemcgaw
Copy link
Member

All other devs/ users who have not migrated yet to .NET 7 and when they migrate will also discover that an accidental F3 will start sorting the column. This is far from being over.

Actually, we do have an option to warn devs migrating right away that they may need to consider an opt out. @pchaurasia14 let's talk about the Upgrade Assistant and what it might be able to help WPF devs migrating from Framework to know right up front. We're using it to warn that we removed certain types, and a few other things. @OliaG is driving and I think it's a great option here.

@karelz karelz added this to the Future milestone Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Product bug (most likely)
Projects
None yet
Development

No branches or pull requests

8 participants