dotnet / winforms

Windows Forms is a .NET UI framework for building Windows desktop applications.
MIT License
4.41k stars 984 forks source link

[API Proposal]: New overload on DataGridView.Sort to specify sort direction for column sort glyph icon #8942

Open elachlan opened 1 year ago

elachlan commented 1 year ago

Background and motivation

Users who implement their own sort logic on a column with SortMode=DataGridViewColumnSortMode.Programmatic find it unintuitive/difficult to set a sort glyph icon.

See issue: #1063

This would allow a user to specify the sort glyph icon direction

API Proposal

namespace System.Window.Forms;

public class DataGridView
{
    // Existing API
    public virtual void Sort(DataGridViewColumn dataGridViewColumn, ListSortDirection direction);

    // New API
    public void Sort (DataGridViewColumn dataGridViewColumn, ListSortDirection direction, SortOrder sortGlyphDirection);
    public void Sort (IComparer comparer, DataGridViewColumn dataGridViewColumn, SortOrder sortGlyphDirection);
}

API Usage

// Sort the gridview
dataGridView1.Sort(dataGridView1.Columns[0], ListSortDirection.Ascending, SortOrder.Descending);

Existing method to achieve same outcome

DataGridViewColumn sortColumn = dataGridView1.Columns[0];
dataGridView1.Sort(sortColumn, ListSortDirection.Ascending);
sortColumn.HeaderCell.SortGlyphDirection = SortOrder;

Alternative Designs

Instead of an overload, add the sortGlyphDirection as an optional parameter with a default value of SortOrder.None.

Risks

Could break existing application logic if optional sortGlyphDirection is used. But usually the glyph is set after you have sorted the gridview.

Will this feature affect UI controls?

Yes, DataGridView will be affected.

elachlan commented 1 year ago

CC: @paul1956 @merriemcgaw

elachlan commented 1 year ago

I don't think the API change makes sense for the IComparer overload.

JeremyKuhne commented 1 year ago

@elachlan I don't think you can make this virtual without breaking existing code as the existing Sort methods are virtual. Derived classes that are expecting to completely override Sort would no longer be doing so. We can't add another parameter to the existing methods as that would be a binary breaking change.

In any case, it would be good to show the existing APIs in the proposal and also illustrate what is currently needed to be written to accomplish this task (without a new API).

elachlan commented 1 year ago

I've updated it. I am not fixated on this specific API design and I am happy to alter it.

I also think maybe we can add:

public void Sort (IComparer comparer, DataGridViewColumn dataGridViewColumn, SortOrder sortGlyphDirection);
elachlan commented 1 year ago

@JeremyKuhne please let me know what needs changing to get this API Proposal to a "ready for review" state.

JeremyKuhne commented 1 year ago

I think we have what we need to discuss the proposal in our next team meeting. If we agree we can add it to the API review backlog then.

KlausLoeffelmann commented 1 year ago

I'd appreciate a small mocked demo (derive DataGridView and extend it for the sample) showing the usage of this.

JeremyKuhne commented 1 year ago

@elachlan is there any reason you need to specify the sort order? Would it be possible to implement this as

public void Sort (DataGridViewColumn dataGridViewColumn, ListSortDirection direction, bool showSortGlyph);
paul1956 commented 1 year ago

@JeremyKuhne the problem I am having is I am passing in a presorted (High to Low) list; All I want to do it have the Glyph show up (correctly) and if it's clicked on allow me to reverse the sort and show the opposite Glyph. Either I get no Glyph, or the Glyph is backwards until it is click on (twice) and then from then on it is correct. Any API that lets me specify the Glyph direction and visibility is OK. It's possible that I just don't understand the API but looking online and the title of this issue I am not the only one. I think what you are proposing is what was originally asked for but for some reason was changed.

elachlan commented 1 year ago

There are currently two sort methods. I think both having overloads with showSortGlyph would help solve the issue.

JeremyKuhne commented 1 year ago

@paul1956, @elachlan what I want to know explicitly is whether or not you can always (or the vast majority of the time) know the right SortOrder based on the specified ListSortDirection. If so, the API would be much clearer if it took a bool. At first glance it seems odd to give a ListSortDirection then a random SortOrder. That said, I'm not super familiar with this.

If the API can be bool then we should update the proposal and explicitly describe what happens for each ListSortDirection.

elachlan commented 1 year ago

We can. But we won't know for IComparable. It would need to be explicit.

paul1956 commented 1 year ago

@JeremyKuhne SortOrder can be None and the column could be sorted in either direction or not sorted.

ghost commented 1 year ago

This submission has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 14 days.

It will be closed if no further activity occurs within 7 days of this comment.

elachlan commented 1 year ago

@JeremyKuhne What do you need from me here? I believe a bool that is used to establish the SortOrder from the ListSortDirection doesn't give the best flexibility.

Do I need to update the proposal body?

merriemcgaw commented 1 year ago

Yes, if you update the proposal body that would be great. Re-add the "untriaged" label when you're ready for us to do a final review and we'll submit to the API Review board if you'd like.

elachlan commented 1 year ago

@merriemcgaw can you please re-review this and add to .NET9?