Jumping between records

[SarahRDBingo]

Update 1.1.10 with the addition of a jumpy scroll patch has introduced a new record jumping bug. When adding a new record and changing the concept name of the record, the program will toggle between the previous record and the newly created record, selection flips back and forth between the two records as if the program is stuck in a loop.

[hohonuuli]

This was also reported by MBARI's video lab. I rolled back changes that may be triggering this. @SarahRDBingo Try release 1.1.11.

[hohonuuli]

Leaving this issue open until I hear that it's resolved.

[SarahRDBingo]

@hohonuuli jumping between records issue is resolved in 1.1.11. Previous jumping scroll issue still exists.

[hohonuuli]

When there are no associations the scrollTo behavior works as expected. When there are many associations, we see the jumping. The upshot is that rows with associations have variable height. The JavaFX TableView appears to be assuming that each row has a fixed height. (The tableview only renders the visible rows, so it doesn't actually know what the height of every row is).

[hohonuuli]

There's a conversation about this in the openjfx dev mailing list at https://mail.openjdk.java.net/pipermail/openjfx-dev/2022-April/034020.html

[hohonuuli]

PR in JavaFX that might finally fix this ... https://github.com/openjdk/jfx/pull/712

[hohonuuli]

New PR is JavaFX related to this ... openjdk/jfx#808

[hohonuuli]

From openjfx email thread:

OpenJFX 17 contains a fix for JDK-8089589 [1] ([ListView] ScrollBar content moves toward-backward during scrolling.) Before this fix, the scrollbar in the VirtualFlow backing List/Table/TreeTableView controls was treating every cell of equal height. That generated an awkward scrolling experience, especially when there are major differences in sizes in the backing list.

The original fix introduced some new issues, and most of these are meanwhile fixed, and tests are added (thanks to e.g. Florian Kirmaier who created a good amount of tests).

However, people have in the past been relying on undocumented behavior while creating apps and libraries, and that behavior is not guaranteed anymore with the fix for JDK-8089589 that went into OpenJFX 17. Most of these cases are related to (1) assuming that IndexedCell.updateItem() is only called when an item is (about to be) shown and (2) assuming that the value returned by the positionProperty of the scrollbar thumb is linearly related to the index of the item in the backinglist being shown.

Both of these assumptions are invalid, but were (more or less) happening before JavaFX 17. I have seen a fair amount of creative implementations that somehow get and process information about the position of elements in the controls. Some of these relied on the false assumptions above, and are obviously no longer working. Instead of providing fixes per case, I think it would be good to capture the common use cases, and make it easier for developers to achieve what they want to achieve. I didn't yet encounter a use case that can not be implemented with the current approach (using public API's only), but granted, implementations often require too much boilerplate and wiring code.

Crucial in this exercise are tests. There are a fair amount of tests for the list/table/treetableview controls, but only few of them are focused on external access and behavior. For all the fixes done so far in VirtualFlow, additional tests have been written which are now excellent regression tests. The last thing we want is that a fix for usecase A is not working anymore after a fix for usecase B.

Hence, I'd like us to do 2 things: 1) for issues that are clearly bugs (violating the JavaDoc), please create JBS issues as usual (with failing test code). 2) for issues that are not violating the specs now, but are "common sense", please create a description and test scenario with expected versus actual outcome.

I am not 100% sure what the best approach is to tackle issues that fall in the second category, but I suggest this: if you have a rather vague, abstract issue (e.g. "listview should always scroll to the bottom of the list IF it was at the bottom before and a new element is added") , it probably makes sense to first discuss it here. If on the other hand you have a very clear issue with a clear failing test, it can be a JBS issue as well -- but we need to keep in mind then that it might be in the category of "this fixes usescase A but blocks usecase B"

Thanks,

• Johan

[1] https://bugs.openjdk.org/browse/JDK-8089589

[hohonuuli]

Video lab reported on this jumping issue today. They are reporting that it occurs even if there are no associations (all rows are the same height). So it might not be related to variable sized rows due to varying numbers of associations on each annotation. Problem is partially mitigated if they sort the table in reverse chronological order. (Then the new annotations appear at the top).

Explore this further. May have to try out alternatives to javafx's TableView control.

[hohonuuli]

Maybe investigate using https://github.com/palexdev/MaterialFX

[hohonuuli]

From Kris Walz:

I reached 325 annotations in the VARS table and it started doing the issue where all other annotations disappear in table except the most recent one you have made, see attached screenshot. And as Megan said yesterday, it's writing each new annotation at bottom every time but the rest of table is staying blank (I had an older version of VARS on my laptop that was rotating between write them all then this blank issue). Note, there are no associations near this annotation so all the cells are the same size.

If I reverse the Date/Time field, it draws table correctly each time I make an annotation, see screen shot 2. Just wanted to send along some visuals to go with our description in meeting yesterday.

[hohonuuli]

Adding a new column # Details so users can turn off the Details column but still get feedback on the number of associations. The fixed row height may resolve some of the jumping issues.

[hohonuuli]

@SarahRDBingo Can you and your lab do some testing for me. There's a new release of VARS at https://github.com/mbari-media-management/vars-annotation/releases/tag/1.4.0-rc2 that has a new column. In the table hide the Details column and make sure the # Details column is visible (reminder: show/hide columns using the plus on the upper right of the table). This will make all the rows the same height, I think variable height rows is contributing to this problem. You won't see the associations for every row at once but it will give you feedback on associations. After you try it out a bit woudl you:

• let me know if that improves the table jumping
• Not seeing all associations, except when the rows is selected, is workable for you.
• Any other UI suggestions that might help.

Thanks!

[SarahRDBingo]

@hohonuuli I used the new 1.4.0-rc2 release. Reducing the row height to a single value seems to have alleviated the issue of scroll jumping around on the table. The issue #129 problem of concept name changing also seems to have been reduced, but it still occurs. I think the hiding of the details list in the table will make it difficult to spot when records are missing important information. It's workable for now, but I will have to get used to checking the editor window for the contents of the details. It is challenging to compare details between records, and it increases the number of clicks I make to look through the details. I think that setting a fixed row height is ideal, however I would still like to be able to see the details in the annotation table. What if the row height was set, but leave the details column in. If the extra details that extend beyond the height of the row are cut off, it's okay. Currently the details column has a little bit of a scroll capability so you can see what extends beyond the height of the row.

[hohonuuli]

From Kris:

I'm working with a file with over 300 lines of annotation (only one has 1 association) and the blank table happened for this version of VARS as well, see attached. I removed the Details field and only have the # Details showing as you outlined below and this doesn't correct the issue. Here's the filename if you need it for testing: D788_20150808T155638.866Z_t4s1_hd_tc02245000_prores.mov (could also use the mp4).

[hohonuuli]

I think I'll try integrating the swing table from the old vars and seeing if it's performance is acceptable. I'll need to add a mthod to add/remove columns from the table model so that it's behavior is closer to the JavaFx TableView. I found this code to allow that:

TableColumnManager.java

import java.awt.*;
import java.beans.*;
import java.util.ArrayList;
import java.util.List;
import java.awt.event.*;
import javax.swing.*;
import javax.swing.event.*;
import javax.swing.table.*;

/**
 *  The TableColumnManager can be used to manage TableColumns. It will give the
 *  user the ability to hide columns and then reshow them in their last viewed
 *  position. This functionality is supported by a popup menu added to the
 *  table header of the table. The TableColumnModel is still used to control
 *  the view for the table. The manager will inovoke the appropriate methods
 *  of the TableColumnModel to hide/show columns as required.
 *
 */
public class TableColumnManager
    implements MouseListener, ActionListener, TableColumnModelListener, PropertyChangeListener
{
    private JTable table;
    private TableColumnModel tcm;
    private boolean menuPopup;

    private List<TableColumn> allColumns;

    /**
     *  Convenience constructor for creating a TableColumnManager for a table.
     *  Support for a popup menu on the table header will be enabled.
     *
     *  @param table the table whose TableColumns will managed.
     */
    public TableColumnManager(JTable table)
    {
        this(table, true);
    }

    /**
     *  Create a TableColumnManager for a table.
     *
     *  @param table the table whose TableColumns will managed.
     *  @param menuPopup enable or disable a popup menu to allow the users to
     *                 manager the visibility of TableColumns.
     */
    public TableColumnManager(JTable table, boolean menuPopup)
    {
        this.table = table;
        setMenuPopup( menuPopup );

        table.addPropertyChangeListener( this );
        reset();
    }

    /**
     *  Reset the TableColumnManager to only manage the TableColumns that are
     *  currently visible in the table.
     *
     *  Generally this method should only be invoked by the TableColumnManager
     *  when the TableModel of the table is changed.
     */
    public void reset()
    {
        table.getColumnModel().removeColumnModelListener( this );
        tcm = table.getColumnModel();
        tcm.addColumnModelListener( this );

        //  Keep a duplicate TableColumns for managing hidden TableColumns

        int count = tcm.getColumnCount();
        allColumns = new ArrayList<TableColumn>(count);

        for (int i = 0; i < count; i++)
        {
            allColumns.add( tcm.getColumn( i ) );
        }
    }

    /**
     *  Get the popup support.
     *
     *  @returns the popup support
     */
    public boolean isMenuPopup()
    {
        return menuPopup;
    }

    /**
     *  Add/remove support for a popup menu to the table header. The popup
     *  menu will give the user control over which columns are visible.
     *
     *  @param  menuPopop when true support for displaying a popup menu is added
     *                  otherwise the popup menu is removed.
     */
    public void setMenuPopup(boolean menuPopup)
    {
        table.getTableHeader().removeMouseListener( this );

        if (menuPopup)
            table.getTableHeader().addMouseListener( this );

        this.menuPopup = menuPopup;
    }

    /**
     *  Hide a column from view in the table.
     *
     *  @param  modelColumn  the column index from the TableModel
     *                     of the column to be removed
     */
    public void hideColumn(int modelColumn)
    {
        int viewColumn = table.convertColumnIndexToView( modelColumn );

        if (viewColumn != -1)
        {
            TableColumn column = tcm.getColumn(viewColumn);
            hideColumn(column);
        }
    }

    /**
     *  Hide a column from view in the table.
     *
     *  @param  columnName   the column name of the column to be removed
     */
    public void hideColumn(Object columnName)
    {
        if (columnName == null) return;

        for (int i = 0; i < tcm.getColumnCount(); i++)
        {
            TableColumn column = tcm.getColumn( i );

            if (columnName.equals(column.getHeaderValue()))
            {
                hideColumn( column );
                break;
            }
        }
    }

    /**
     *  Hide a column from view in the table.
     *
     *  @param  column  the TableColumn to be removed from the
     *                TableColumnModel of the table
     */
    public void hideColumn(TableColumn column)
    {
        if (tcm.getColumnCount() == 1) return;

        //  Ignore changes to the TableColumnModel made by the TableColumnManager

        tcm.removeColumnModelListener( this );
        tcm.removeColumn( column );
        tcm.addColumnModelListener( this );
    }

    /**
     *  Show a hidden column in the table.
     *
     *  @param  modelColumn  the column index from the TableModel
     *                     of the column to be added
     */
    public void showColumn(int modelColumn)
    {
        for (TableColumn column : allColumns)
        {
            if (column.getModelIndex() == modelColumn)
            {
                showColumn( column );
                break;
            }
        }
    }

    /**
     *  Show a hidden column in the table.
     *
     *  @param  columnName   the column name from the TableModel
     *                     of the column to be added
     */
    public void showColumn(Object columnName)
    {
        for (TableColumn column : allColumns)
        {
            if (column.getHeaderValue().equals(columnName))
            {
                showColumn( column );
                break;
            }
        }
    }

    /**
     *  Show a hidden column in the table. The column will be positioned
     *  at its proper place in the view of the table.
     *
     *  @param  column   the TableColumn to be shown.
     */
    private void showColumn(TableColumn column)
    {
        //  Ignore changes to the TableColumnModel made by the TableColumnManager

        tcm.removeColumnModelListener( this );

        //  Add the column to the end of the table

        tcm.addColumn( column );

        //  Move the column to its position before it was hidden.
        //  (Multiple columns may be hidden so we need to find the first
        //  visible column before this column so the column can be moved
        //  to the appropriate position)

        int position = allColumns.indexOf( column );
        int from = tcm.getColumnCount() - 1;
        int to = 0;

        for (int i = position - 1; i > -1; i--)
        {
            try
            {
                TableColumn visibleColumn = allColumns.get( i );
                to = tcm.getColumnIndex( visibleColumn.getHeaderValue() ) + 1;
                break;
            }
            catch(IllegalArgumentException e) {}
        }

        tcm.moveColumn(from, to);

        tcm.addColumnModelListener( this );
    }
//
//  Implement MouseListener
//
    public void mousePressed(MouseEvent e)
    {
        checkForPopup( e );
    }

    public void mouseReleased(MouseEvent e)
    {
        checkForPopup( e );
    }

    public void mouseClicked(MouseEvent e) {}
    public void mouseEntered(MouseEvent e) {}
    public void mouseExited(MouseEvent e) {}

    private void checkForPopup(MouseEvent e)
    {
        if (e.isPopupTrigger())
        {
            JTableHeader header = (JTableHeader)e.getComponent();
            int column = header.columnAtPoint( e.getPoint() );
            showPopup(column);
        }
    }

    /*
     *  Show a popup containing items for all the columns found in the
     *  table column manager. The popup will be displayed below the table
     *  header columns that was clicked.
     *
     *  @param  index  index of the table header column that was clicked
     */
    private void showPopup(int index)
    {
        Object headerValue = tcm.getColumn( index ).getHeaderValue();
        int columnCount = tcm.getColumnCount();
        JPopupMenu popup = new SelectPopupMenu();

        //  Create a menu item for all columns managed by the table column
        //  manager, checking to see if the column is shown or hidden.

        for (TableColumn tableColumn : allColumns)
        {
            Object value = tableColumn.getHeaderValue();
            JCheckBoxMenuItem item = new JCheckBoxMenuItem( value.toString() );
            item.addActionListener( this );

            try
            {
                tcm.getColumnIndex( value );
                item.setSelected( true );

                if (columnCount == 1)
                    item.setEnabled( false );
            }
            catch(IllegalArgumentException e)
            {
                item.setSelected( false );
            }

            popup.add( item );

            if (value == headerValue)
                popup.setSelected( item );
        }

        //  Display the popup below the TableHeader

        JTableHeader header = table.getTableHeader();
        Rectangle r = header.getHeaderRect( index );
        popup.show(header, r.x, r.height);
    }
//
//  Implement ActionListener
//
    /*
     *  A table column will either be added to the table or removed from the
     *  table depending on the state of the menu item that was clicked.
     */
    public void actionPerformed(ActionEvent event)
    {
        if (event.getSource() instanceof AbstractButton)
        {
            AbstractButton button = (AbstractButton)event.getSource();
            String column = event.getActionCommand();

            if (button.isSelected())
                showColumn(column);
            else
                hideColumn(column);
        }
    }
//
//  Implement TableColumnModelListener
//
    public void columnAdded(TableColumnModelEvent e)
    {
        //  A table column was added to the TableColumnModel so we need
        //  to update the manager to track this column

        TableColumn column = tcm.getColumn( e.getToIndex() );

        if (allColumns.contains( column ))
            return;
        else
            allColumns.add( column );
    }

    public void columnMoved(TableColumnModelEvent e)
    {
        if (e.getFromIndex() == e.getToIndex()) return;

        //  A table column has been moved one position to the left or right
        //  in the view of the table so we need to update the manager to
        //  track the new location

        int index = e.getToIndex();
        TableColumn column = tcm.getColumn( index );
        allColumns.remove( column );

        if (index == 0)
        {
            allColumns.add(0, column);
        }
        else
        {
            index--;
            TableColumn visibleColumn = tcm.getColumn( index );
            int insertionColumn = allColumns.indexOf( visibleColumn );
            allColumns.add(insertionColumn + 1, column);
        }
    }

    public void columnMarginChanged(ChangeEvent e) {}
    public void columnRemoved(TableColumnModelEvent e) {}
    public void columnSelectionChanged(ListSelectionEvent e) {}
//
//  Implement PropertyChangeListener
//
    public void propertyChange(PropertyChangeEvent e)
    {
        if ("model".equals(e.getPropertyName()))
        {
            if (table.getAutoCreateColumnsFromModel())
                reset();
        }
    }

    /*
     *  Allows you to select a specific menu item when the popup is
     *  displayed. (ie. this is a bug? fix)
     */
    class SelectPopupMenu extends JPopupMenu
    {
        @Override
        public void setSelected(Component sel)
        {
            int index = getComponentIndex( sel );
            getSelectionModel().setSelectedIndex(index);
            final MenuElement me[] = new MenuElement[2];
            me[0]=(MenuElement)this;
            me[1]=getSubElements()[index];

            SwingUtilities.invokeLater(new Runnable()
            {
                public void run()
                {
                    MenuSelectionManager.defaultManager().setSelectedPath(me);
                }
            });
        }
    };
}

[hohonuuli]

@SarahRDBingo I've put out another VARS release for you at https://github.com/mbari-media-management/vars-annotation/releases/tag/1.4.0-rc3. I'm feeling hopeful about this one ... can you try using this in your usual mode, with the details/associations shown in the table view. As always feedback is appreciated.

[SarahRDBingo]

@SarahRDBingo I've put out another VARS release for you at https://github.com/mbari-media-management/vars-annotation/releases/tag/1.4.0-rc3. I'm feeling hopeful about this one ... can you try using this in your usual mode, with the details/associations shown in the table view. As always feedback is appreciated.

@hohonuuli I used the new test release version for a full day. Unfortunately the scroll jump still occurs whenever adding/removing a new line to the annotation table. If I use the GUI buttons for copy time, copy annotation, or new annotation, the position of the table will temporarily jump away from the newly selected row. When the new concept name is added, the table jumps back to the row being edited. I haven't experienced the scroll jump as I add details to an annotation record. The table view will also occasionally freeze or become unresponsive when trying to scroll through the table after adding a number of details. The only thing that seems to break the freeze is to change the shape/size of the annotation table window. I am using the original details column rather than the "Details #" column.

[hohonuuli]

I've started work on a Swing implementation (how the old VARS used to render the table). It's done in the feature/swingtable branch. Initial work added. some stuff that needs to be done:

• table selection is not propagating correctly yet
• scrollbar is not sized correctly. May have to tie size of JScrollPane to bounds of JavaFX parent container.
• store/load preferences: hidden columns, column widths, column order.
• Association Column is not resizing the row vertically as expected.

Things to be mindful:

• TableModel always returns a reference to the annotation regardless of the column index. This allows row order to be changed without messing up the view.

[hohonuuli]

Notes for myself: I pushed changes to the TableView's items property. In JavaFX 8, how sorting was handled changed. I missed that bit in the javadocs. Following the recommendation, there's a backing model tableItems that we use for mutations. However the table wraps that in a SortedList and binds the sorted lists comparator to the tableview's comparator.

This change removes the calls previously need to refresh (to redraw the table) and to sort (to maintain correct sort order in the view). I push a new release on Monday for testing

[MeaganPutts]

@hohonuuli I tested out vars-annotation 1.4.0-rc5. The jumping and lagging are still present and seems to occur more often than last week. Jumps and lags still occur when details are turned off.

129 This still remains an issue and is worse today than when I was annotating last week.

[hohonuuli]

@MeaganPutts OK, thanks for testing it out. You can roll back to an earlier version for now. It looks like I'll have to pursue a tableview replacement (started in feature/swingview)

[hohonuuli]

Some more clues in this bug

Also a note from the JavaFX mailing list from today:

Update: I had a quick look, downloaded the code and for a single scroll event, there are 74 calls to updateItem. However, 69 of those are invoked via com.jfoenix.skins.JFXListViewSkin.estimateHeight which calls com.jfoenix.adapters.VirtualFlowHelper.forEach which calls into VirtualFlow.setCell itself.

This sounds like something to discuss with the maintainers of the library (jfoenix I believe). Maybe that library tries estimating the height of the flow as well?

VARS is using the jfoenix library. I need to double check to see if it's used in the table.

[hohonuuli]

Just checked that we are no using jfoenix widgets in the tableview.

[MeaganPutts]

@hohonuuli I started using vars-annotation 1.4.0-rc6 release yesterday. The jumping when records are added/deleted is reduced but the changing of a record to a previous concept name still persists. However, the biggest issue I have with the new release is that I can no longer take screen grabs. I double checked that my settings are correct and I was able to screen grab without issue in the previous release just minutes before downloading 1.4.0-rc6. When I click on the screen grab button nothing happens, in the past when the screen grab failed, I received an error message.

[hohonuuli]

@MeaganPutts Thanks for letting me know. Our video lab also reported that too. I'll look into a fix tomorrow.

[hohonuuli]

This may be related to https://github.com/openjdk/jfx/pull/974

[hohonuuli]

However, the biggest issue I have with the new release is that I can no longer take screen grabs.

This is fixed in 1.4.0-rc7

[hohonuuli]

Wrote a replacement of the table view using Swing. Need to figure out how to save the column info (shown/hidden, order, width) to preferences. Not clear yet how to access this info from the table control.

[hohonuuli]

This may be addressed in release 1.5.0 🤞

[hohonuuli]

From @SarahRDBingo:

VARS 1.5.0 is a major improvement with the stability of the listview as records are added/removed/edited.

[hohonuuli]

@MeaganPutts Reported that this may be resolved now in (Finally) #156