Sorting out Sorting

alanjfpaterson

I started off working on the Daily Exchange Rate screen, but it has turned into an interesting exercise in debugging some issues about sorting. And this all arose because this screen in particular is sorted right from the beginning, whereas most other screens are displayed in record-addition order.

So it has turned out that there was less wrong with the Daily Exchange Rate screen than we first thought. Several of the unexpected behaviours were caused by much lower level code that is common to many screens - you just don't realise because we do not often click on the sort headers and then start using the screens. If we did, these bugs would be everywhere!!

First, I found that on some screens, sorting resulted in some rows becoming duplicates of the 'current' row at the start of the sort - so if you repeatedly clicked on the sort header bar you could end up with multiple rows the same (and so of course rows that had been there were now missing because the number of rows does not change). This only occurs when the primary key involves multiple columns and you sort a column that has multiple rows with the same column value (but different values in other columns to keep uniqueness). Once I managed to work out the order that events were being fired, it was not very difficult to fix this bug for all screens.

Second, I found that clicking the New button when a column was sorted could also give an unexpected behaviour on every screen we have! Let me explain... All our screens are coded so that a new row will have a predictable new row added. Usually this new row is added at the bottom of the table because the rows, by default, are shown in record addition order. If, however, you have clicked the sort header, adding a new record will quite likely display the newly added row somewhere in the middle of the grid. This is quite correct. Now, take any screen you like, sort the grid and position the selected row on the row where the New row will be inserted (above). Now click New. The new data will be shown in the grid correctly as you would expect - but the details in the bottom panel will still show the details from the previously selected row. This happens because the selected row has not changed - the new row has replaced the old row and all the other rows have been shuffled down by one. So we never fired the event that would update the details to match the new row. I would predict that you will pretty much see this on any screen you like to choose. Every sorted grid will have just one specific row selection where this happens. All other row selections will work fine. Again, this is not a difficult fix to make, but what I initially thought was a Daily Exchange Rate screen issue turned out to be more generic; but easier to spot because data in this screen is always sorted.

Third, I wanted to include data validation on two fields in the screen - it has a date box so that needs the same validation we have put in to many other screens; I also wanted to have validation that the entered exchange rate was not zero (since we offer to invert it, and I hope that not even Greece has a zero exchange rate). I was interested to see how sorting and validation interacted with one another and the order of events between the two. Oh dear! Another case where the validation works and I get the tool-tips as I expect, but the data in the grid changes and rows that are nothing to do with the current row start getting modified data in them. I have not finished getting to the bottom of this, because I reached the end of the day. So this will be my next place to start.

But it has been very interesting to see how we really need to check carefully whether the code we have written really supports sorting. First, does the act of sorting change any data? Second does working with already sorted data change the way that our buttons work? The really scary thing is that data can get changed quite subtly - just one row in a large grid for example, and unless you look carefully you don't notice but when you save, that data is saved (unless a constraint is violated, which might appear initially odd), and real (correct) data is lost. Definitely something for testers to bear in mind.

I will continue with making these fixes this week and then et back to the specifics of the Daily Exchange rate screen.

wbrowa

Thanks for your investigations, Alan. I was aware of the sorting issue and that it is something we need to work on as I ran into it with other screens. As I did not follow your other discussion I did not realize that it has implications with your work. Please let us know how you fix it for this screen so we can adapt the other screens (any screen with a grid really) or much better the templates or central grid methods.

Thanks,
Wolfgang

alanjfpaterson

I thought I would conclude (?) this post by showing the code changes that I have made and explaining what the changes actually do. This has some relevance to what Chris is doing, trying to keep on top of RowChanged events and ensuring that we do not fire duplicate events un-necessessarily. Some of the techniques I have used may be useful elsewhere, because getting events in the right order is tricky.

Before I go into detail, you need to understand that there are two critical form-level variables that need to be constantly up to date and in synch: FPreviouslySelectedDetailRow is the DataRow object that is usually our current row. It is the dataRow that we are working on. Then FCurrentRow is the actual integer row in the grid that is the one that corresponds to that. We must be sure never to let these get mismatched. You might think this is quite easy - but believe me - not when the grid is sorted, either manually by clicking on the row header, or programatically as a requirement of the screen.

The first issue I resolved was to do with clicking the sort header. This causes the data grid to do a lot of work and fire events - but the key thing is that the current data row remains unchanged. The grid works out the new position and selects it. As a result we get a FocusRowChanged event, which contains a vital piece of information - the new row number, which we need to ensure is safely stored in FCurrentRow. Apart from that the goal is to not change anything in the grid or the details panel because whatever the grid does with re-ordering data rows we will end up displaying the same data. So the code that handles this is here.

    private void FocusedRowChanged(System.Object sender, SourceGrid.RowEventArgs e)
    {
        if(e.Row != FCurrentRow && !grdDetails.Sorting)
        {
            // Transfer data from Controls into the DataTable
            if (FPreviouslySelectedDetailRow != null)
            {
                GetDetailsFromControls(FPreviouslySelectedDetailRow);
            }

        // Display the details of the currently selected Row
        FPreviouslySelectedDetailRow = GetSelectedDetailRow();
        ShowDetails(FPreviouslySelectedDetailRow);
        pnlDetails.Enabled = true;
    }
    FCurrentRow = e.Row;
}

private void GetDetailsFromControls(ADailyExchangeRateRow ARow)
{
    if (ARow != null && !grdDetails.Sorting)
    {
        ARow.BeginEdit();
        ARow.FromCurrencyCode = cmbDetailFromCurrencyCode.GetSelectedString();
        ARow.ToCurrencyCode = cmbDetailToCurrencyCode.GetSelectedString();
        ARow.DateEffectiveFrom = (dtpDetailDateEffectiveFrom.Date.HasValue?dtpDetailDateEffectiveFrom.Date.Value:DateTime.MinValue);
        ARow.RateOfExchange = Convert.ToDecimal(txtDetailRateOfExchange.NumberValueDecimal);
        GetDetailDataFromControlsManual(ARow);
        ARow.EndEdit();
    }
}

This uses a new property of the grid - grdDetails.Sorting which is true if the grid is doing its sorting algorithm. As you can see, if the grid is sorting, the only thing we do is update FCurrentRow. We neither supply any details to the grid (from GetDetailsFromControls), nor do we change any details in the details pane as a result of the row change event (FocusRowChanged). If you supply details to the grid it is highly likely to go into the wrong row, and if you respond to the row changed event you are highly likely to be given the wrong row to get it from!

The second issue arises during Validation. On a sorted grid, validation can cause our data row to move to a new location - but the selection does not change unless we make sure that it follows our data. So the new code that achieves this is here.

    private bool ValidateAllData(bool ARecordChangeVerification, bool AProcessAnyDataValidationErrors)
    {
        bool ReturnValue = false;

    if (FPreviouslySelectedDetailRow != null)
    {
        GetDetailsFromControls(FPreviouslySelectedDetailRow);

        // Remember the current rowID and perform automatic validation of data, based on the DB Table specifications (e.g. 'not null' checks)
        int previousRowNum = FCurrentRow;
        ValidateDataDetailsManual(FPreviouslySelectedDetailRow);

        // Validation might have moved the row, so we need to locate it again, updating our FCurrentRow global variable
        FCurrentRow = grdDetails.DataSourceRowToIndex2(FPreviouslySelectedDetailRow) + 1;
        if (FCurrentRow != previousRowNum)
        {
            // Yes it did move so we need to keep the row selected, without firing off the event that brought us here in the first place!
            grdDetails.Selection.FocusRowLeaving -= new SourceGrid.RowCancelEventHandler(FocusRowLeaving);
            grdDetails.SelectRowInGrid(FCurrentRow);
            grdDetails.Selection.FocusRowLeaving += new SourceGrid.RowCancelEventHandler(FocusRowLeaving);
        }

There are several things to understand here. First we need to get the data from the controls into our current DataRow object.

Then we need to remember what our current row is from FCurrentRow. We need this to test if the row has moved after validation.

Then we do the validation actions.

Then we have to find our row again, setting FCurrentRow to the returned value. This uses a new overload to the DataSourceRowToindex2 method which can take a DataRow as an argument in place of a DataRowView.

If the row of data did actually move to a new location we need to make sure we highlight the new location without firing off any events.

Then the code continues as before.

There is one final quirky thing we have to do on New Records. There is always one row that you can highlight in a sorted grid, that when you add a new row will result in the new row being displayed at the same location in the grid as the row that was highlighted before the New. When this occurs, there will be NO RowChanged event fired and as a result the details panel will still hold the previously displayed data and not the data from the new row. So we have to check if the grid row does change when the new row is added and if it does not we have to simulate a RowChange event. Here is the code.

    public bool CreateNewADailyExchangeRate()
    {
        if(ValidateAllData(true, true))
        {
            int previousGridRow = grdDetails.Selection.ActivePosition.Row;
            ADailyExchangeRateRow NewRow = FMainDS.ADailyExchangeRate.NewRowTyped();
            FMainDS.ADailyExchangeRate.Rows.Add(NewRow);

        FPetraUtilsObject.SetChangedFlag();

        grdDetails.DataSource = new DevAge.ComponentModel.BoundDataView(FMainDS.ADailyExchangeRate.DefaultView);
        grdDetails.Refresh();
        SelectDetailRowByDataTableIndex(FMainDS.ADailyExchangeRate.Rows.Count - 1);
        int currentGridRow = grdDetails.Selection.ActivePosition.Row;
        if (currentGridRow == previousGridRow)
        {
            // The grid must be sorted so the new row is displayed where the old one was.  We will not have received a RowChanged event.
            // We need to enforce showing the new details.
            FPreviouslySelectedDetailRow = GetSelectedDetailRow();
            ShowDetails(FPreviouslySelectedDetailRow);
        }

        return true;
    }
    else
    {
        return false;
    }
}

This code is the same as we have used up to now except that it checks the grid row before the addition (previousGridRow) and gets the grid row after the addition (currentGridRow). If they are the same it calls GetSelectedDetailRow() and sets our Form varaiable to that value and then shows the details for the new row.

None of these changes, as far as I know, will need to fire off any additional events - indeed the whole point of the changes has been to try to stop events being acted upon in such a way as to cascade any new events, and usually they try to take action without firing additional ones. But equally these changes probably do not address any existing issues with multiple events that are nothing to do with quirks of sorted behaviour. If they still exist - and that is what Chris is working on - then we should fix those issues too.

By the way, if you are reading this and are interested, on a different (but related) subject - when I was looking into the data that we submit to the server when we save changes - I did, as Christian thought, confirm that every time we select a row in the grid but do not edit it, we do indeed submit that row to the server as a 'changed' row, even though it contains the same data as before. How often when editing do you think we would select lots of detail rows that we do not change - basically a problem for keyboard users rather than mouse users I guess. But that is another story!