Skip to content

Conversation

@sergeyteleshev
Copy link
Contributor

@sergeyteleshev sergeyteleshev commented Oct 1, 2025

closes https://github.com/dbeaver/pro/issues/6859

Changes:

  1. Added pin/unpin logic
  2. Refactored the columnId logic. Now we have two types of IDs. Visual column id represents the column number on the screen. And dataColIdx, which means the column index in the initial columns array provided to the data grid
  3. Some features need to use visual column ID, some need to use dataColIdx. So used it respectively where required
  4. Removed weird cell focus hack cause everything works fine without it

@sergeyteleshev sergeyteleshev self-assigned this Oct 1, 2025
@sergeyteleshev sergeyteleshev marked this pull request as ready for review October 9, 2025 09:00
return () => {
tableData.editor.action.removeHandler(syncEditor);
};
}, [tableData.editor, selectionAction, handlers, tableData, restoreFocus]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems hacky. When I edited the cell, it moved the focus to the wrong place. I deleted that, and during tests, nothing weird appeared with the cell focus. So this code seems like a redundant, dirty hack. I decided to remove it completely

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't just remove a piece of functionality that you don't understand. This code has a precise meaning:
Sync focus state between the grid component and the grid model. Now it's impossible to manipulate the focus from the model, and probably it broke functionality based on it (value panel, row add/duplicate)

focusSyncRef.current = { colIdx, rowIdx };

this.selectCell({ colIdx, rowIdx });
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in use anywhere. Only in the deleted piece below, so I removed it also

return () => {
tableData.editor.action.removeHandler(syncEditor);
};
}, [tableData.editor, selectionAction, handlers, tableData, restoreFocus]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't just remove a piece of functionality that you don't understand. This code has a precise meaning:
Sync focus state between the grid component and the grid model. Now it's impossible to manipulate the focus from the model, and probably it broke functionality based on it (value panel, row add/duplicate)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants