Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-8187314: All Cells: must show backing data always #1197

Closed
wants to merge 2 commits into from

Conversation

Maran23
Copy link
Member

@Maran23 Maran23 commented Aug 4, 2023

Before, the updateItem method was called with the new value that was committed via commitEdit().
This is problematic as developers may setup a commit handler via setOnEditCommit, which may reject the edit (or change the value otherwise).
We therefore do call the updateItem(-1) which will also call updateItem but with the real underlying value.

Changed and added tests for all 4 cells.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

  • JDK-8187314: All Cells: must show backing data always (Bug - P3)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1197/head:pull/1197
$ git checkout pull/1197

Update a local copy of the PR:
$ git checkout pull/1197
$ git pull https://git.openjdk.org/jfx.git pull/1197/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1197

View PR using the GUI difftool:
$ git pr show -t 1197

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1197.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 4, 2023

👋 Welcome back mhanl! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Ready for review label Aug 4, 2023
@mlbridge
Copy link

mlbridge bot commented Aug 4, 2023

Webrevs

@kevinrushforth
Copy link
Member

This seems reasonable.

@aghaisas can you review it. It will need a second reviewer as well.

/reviewers 2

@openjdk
Copy link

openjdk bot commented Aug 5, 2023

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

* See also: <a href="https://bugs.openjdk.org/browse/JDK-8187314">JDK-8187314</a>.
*/
@Test
public void testEditCommitValueChangeIsReflectedInCell() {
Copy link
Contributor

Choose a reason for hiding this comment

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

this test passes with the fix, fails without as expected.
but for some reason I cannot reproduce the issue with ListView in the MonkeyTester
https://github.com/andy-goryachev-oracle/MonkeyTest

(List View Page -> select 'editable' -> double click to edit, type "update" to save changes, any other text to reject)

@andy-goryachev-oracle
Copy link
Contributor

I've tested this change with ListView, TableView, and TreeView.
Do you have a SCCE for TreeTableView similar to the TreeView example in the ticket?

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 13, 2023

@Maran23 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 11, 2023

@Maran23 This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@bridgekeeper bridgekeeper bot closed this Oct 11, 2023
@Maran23
Copy link
Member Author

Maran23 commented Oct 11, 2023

/open

@openjdk openjdk bot reopened this Oct 11, 2023
@openjdk
Copy link

openjdk bot commented Oct 11, 2023

@Maran23 This pull request is now open

@andy-goryachev-oracle
Copy link
Contributor

Do you have a SCCE for TreeTableView similar to the TreeView example in the ticket?

@Maran23
Copy link
Member Author

Maran23 commented Oct 12, 2023

Do you have a SCCE for TreeTableView similar to the TreeView example in the ticket?

It should be the same as TreeTableView works basically the same as TableView.

@andy-goryachev-oracle
Copy link
Contributor

could you please create one?

@Maran23
Copy link
Member Author

Maran23 commented Oct 19, 2023

As I said, everything is basically the same, but changed to TreeItem:

import javafx.application.Application;
import javafx.collections.FXCollections;
import javafx.collections.ObservableList;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.TreeItem;
import javafx.scene.control.TreeTableColumn;
import javafx.scene.control.TreeTablePosition;
import javafx.scene.control.TreeTableView;
import javafx.scene.control.cell.TextFieldTreeTableCell;
import javafx.scene.control.cell.TreeItemPropertyValueFactory;
import javafx.scene.layout.BorderPane;
import javafx.stage.Stage;

public class Test extends Application {

    TreeTablePosition<Dummy, String> editPosition;
    private Object editValue;
    @Override
    public void start(Stage primaryStage) {
        TreeTableView<Dummy> table = new TreeTableView<>();
        table.setRoot(new TreeItem<>());
        for (Dummy dummy : Dummy.dummies()) {
            TreeItem<Dummy> dummyItem = new TreeItem<>(dummy);

            table.getRoot().getChildren().add(dummyItem);
        }

        table.setEditable(true);

        TreeTableColumn<Dummy, String> first = new TreeTableColumn<>("Text");
        first.setCellFactory(TextFieldTreeTableCell.forTreeTableColumn());
        first.setCellValueFactory(new TreeItemPropertyValueFactory<>("dummy"));

        first.setOnEditStart(t -> editPosition = t.getTreeTablePosition());
        first.addEventHandler(TreeTableColumn.editCommitEvent(), t -> {
            editValue = t.getNewValue();
            System.out.println("doing nothing");

        });

        table.getColumns().addAll(first);

        Button button = new Button("Check value");
        button.setOnAction(e -> {
            if (editPosition == null) return;
            String value = editPosition.getTableColumn().getCellObservableValue(editPosition.getRow()).getValue();
            System.out.println(
                    "value in edited cell must represent backing data: " + value + " not the edited " + editValue);
        });
        BorderPane root = new BorderPane(table);
        root.setBottom(button);
        Scene scene = new Scene(root, 300, 250);

        primaryStage.setTitle("Hello World!");
        primaryStage.setScene(scene);
        primaryStage.show();
    }

    public static void main(String[] args) {
        launch(args);
    }

    public static class Dummy {
        private String dummy;
        public Dummy(String dummy) {
            this.dummy = dummy;
        }

        public String getDummy() {
            return dummy;
        }

        public static ObservableList<Dummy> dummies() {
            return FXCollections.observableArrayList(
                    new Dummy("1"), new Dummy("2"), new Dummy("3")
            );
        }
    }
}

@andy-goryachev-oracle
Copy link
Contributor

Thank you so much, @Maran23 ! It makes it so much easier to review.

Since the master progressed a bit since the last review, may I trouble you to merge in the latest master please?

@andy-goryachev-oracle
Copy link
Contributor

Also, would it be possible to extend the tree table view test to include a second column with the same logic?

@Maran23
Copy link
Member Author

Maran23 commented Oct 20, 2023

Also, would it be possible to extend the tree table view test to include a second column with the same logic?

Sure, but why?
I mean, a test should usually consist of as few components as are needed for testing, since we want to test a specific problem (in isolation).
And one column is sufficient to test the issue, more columns will just add more bloat but no new insights (and the issue is not related to a particular column anyway)

@andy-goryachev-oracle
Copy link
Contributor

Sure, but why?

The first column in tree table is different from subsequent columns. I just want to make sure it works there as well.

@andy-goryachev-oracle
Copy link
Contributor

TreeTableView with multiple columns:

https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/bugs/TreeTableView_8187314.java

works as expected.

Copy link
Contributor

@andy-goryachev-oracle andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

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

🕶 looks good, thanks!

@openjdk
Copy link

openjdk bot commented Oct 31, 2023

@Maran23 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8187314: All Cells: must show backing data always

Reviewed-by: angorya, aghaisas

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Ready to be integrated label Oct 31, 2023
@Maran23
Copy link
Member Author

Maran23 commented Nov 2, 2023

TreeTableView with multiple columns works as expected.

Thanks for checking. In the meantime I also checked out the tests, and all of them do test one column (startEdit, cancelEdit, commitEdit, setOnEditCommit, ....), so I really think we are good here! 🙂

@Maran23
Copy link
Member Author

Maran23 commented Nov 2, 2023

/integrate

@openjdk
Copy link

openjdk bot commented Nov 2, 2023

Going to push as commit ead1953.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 2, 2023
@openjdk openjdk bot closed this Nov 2, 2023
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review labels Nov 2, 2023
@openjdk
Copy link

openjdk bot commented Nov 2, 2023

@Maran23 Pushed as commit ead1953.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
4 participants