Open RajeshAKumar opened 1 year ago
/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.
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?
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.
We also support user's ability to sort via clicking on column headers. So no, we cannot set CanUserSort to false.
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);
}
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'.
Please refer to the discussion in #6737 how this got in despite it being a breaking change.
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.
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?
@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.
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.
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.
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.
It may not help hindsight, but up the chain I want folks to start thinking of "Stability is > Speed of delivery" which is what MS had for long. I started on Windows 3.1 and Visual Studio 1.0 on desktop platform and of about 25+ years of VS development, I did not find many such breaking changes on applications, that might impact end users. I am a staunch supporter of MS and dev eco system that MS provides, so definitely would want the "Design, approve, Implement, Test, Code Review, Impact Review" cycle be more compliant, especially if there are >1000 users of your product.
Breaking changes for developers, I can understand those, not an issue. Compiler warnings/ errors will take care of it.
Not sure why Win Forms Dev is even suggesting things on WPF controls, not sure how Sr WPF Eng Mgr approves of these changes. In our organization such changes would case a write-up on the person and few of them will be talked about in next perf review.
@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.
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 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.
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.
@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 - 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.
@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 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.
Any update on the release of this?
The fix is already merged for .NET 7 servicing release. Should be available in upcoming 7.0.3 release.
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.
I tried this with 7.0.4 and the issue still exists.
@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.
Add the following code to your .csproj
file.
<Project>
....
<ItemGroup>
<!-- App Context Switch-->
<RuntimeHostConfigurationOption Include="System.Windows.Controls.DisableDataGridKeyboardSort" Value="true"/>
</ItemGroup>
....
</Project>
Let me know if any step is unclear or confusing. grid.zip
@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!
@pchaurasia14 @merriemcgaw : Any thoughts on how to prevent this breaking change out of the box? Please see my comments above
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.
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.
@RajeshAKumar -
I did not know of the App-Context switch.
We tried to communicate it as clearly as possible - see https://github.com/dotnet/wpf/issues/7288#issuecomment-1314781631 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.
@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.
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.
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?