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

Fixes issue #9031 - Failed attempt 1 #9326

Closed

Conversation

YuiDOS
Copy link

@YuiDOS YuiDOS commented Oct 30, 2022

Please note this is a failed attempt tried to fix #9031. The PR is opened for recording the attempt.

Positioning the problem
We thought the issue is related to createSpecialColumn method in SpecialFieldColumn.java, because we found a listener which listen double click of primary button of mouse and single click of secondary button. This listener looks quite relevant to the issue.

Attempt
We then modified the method, tried to get the past value (which actually does not exist in the method) of ranking field and compare it with the new value of ranking field in the method. What's more, we also only changed the number of required click to perform clear ranking from 2 to 1. This seems could fix the issue (but actually it did not).

Actual behavior
Per the issue description, the expected behavior of the modified application is: if the single click on the same ranking as past ranking is detected, then the ranking is cleared, per the issue description.
The actual behavior is: while single-clicking on any of five stars (even when clicking the not same ranking star as the past ranking), the ranking will be cleared. The below gif animation provides a direct view of the actual behavior:

recording

Actions performed in the animation:
Single-click on the third star individually twice,
Single-click on the fourth star individually twice,
Single-click on the third star once, then single-click on the fifth star once.

Testing
We did not perform any kind of automatic testing. Only the above manual testing was performed.
There might be potential influence (bug) to other part of the code.

Analyzing the attempt
From the attempt, we can see that the modified application did not performed as the expected behavior indicated in the issue description.
The main problem we found in this attempt is that we can't find the past value of ranking of that entry. The argument 'entry', after our testing, we found it is always already updated its ranking value, so we can't see any way to only modify the createSpecialColumn method to fix the issue.
We asked questions about this issue, and Carl replied that the createSpecialColumn method is this method 'should only be called once on initialization of the table and the ranking control', 'createSpecialRating delivers the JavaFX node to be displayed in the TableView' and is just a draw method.

What's next?
There are several ways to continue working on this issue:

  1. Research 'TableView' mentioned above,
  2. When we single-click on a star on a clear-rank, the ranking is being set. Finding this listener might be helpful.
  3. RatingSkin.java seems worth to have a look at.
  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@ThiloteE ThiloteE changed the title Fixes issue#9031 - failed attempt 1 Fixes issue #9031 - Failed attempt 1 Oct 30, 2022
@calixtus
Copy link
Member

Solution will probably to alter the default behaviour of the controlsfx ranking control.

@calixtus
Copy link
Member

Thanks for opening this PR with the detailed information of your failed attempt. We always learn from our mistakes.

@calixtus calixtus closed this Oct 30, 2022
@Siedlerchr
Copy link
Member

@YuiDOS Maybe you can open an issue in the controlsfx repo?

@SebieF
Copy link
Contributor

SebieF commented Nov 1, 2022

@YuiDOS During my investigation of #9334 I was directed to this issue and I looked into it a bit.
I think the main problem here is that in SpecialFieldColumn, the binding will not be "fired", because setting a property to the same value as before in JavaFX will not activate any observers. This is intended but a problem in our case, where we do want to react to a click on the same star, i.e. the same ranking.

This is the code I am talking about:

EasyBind.subscribe(ranking.ratingProperty(), rating ->
                new SpecialFieldViewModel(SpecialField.RANKING, preferencesService, undoManager)
                        .setSpecialFieldValue(entry.getEntry(), SpecialFieldValue.getRating(rating.intValue())));

I think there is no easy way around this without hurting the design principles of JavaFX properties (but I am not sure here). However, I found a very hacky workaround using the event filter function, that you also mentioned in your PR:

        ranking.addEventFilter(MouseEvent.MOUSE_CLICKED, event -> {

            if (event.getButton().equals(MouseButton.PRIMARY) && event.getClickCount() == 2) {
                ranking.setRating(0);
                event.consume();
            } else if (event.getButton().equals(MouseButton.PRIMARY) && event.getClickCount() == 1) {
                    if(entry.getEntry().getField(SpecialField.RANKING).isPresent()) {
                        // Get the position of the clicked star (1-5)
                        var clickedStar = ((FontIcon) event.getTarget());
                        var parent = (HBox) clickedStar.getParent();
                        var positionOfStarInParent = parent.getChildren().indexOf(clickedStar) + 1;
                        
                        // Compare clicked with previous rating
                        var clickedRating = SpecialFieldValue.getRating(positionOfStarInParent);
                        var previousRating = SpecialFieldValue.getRating(
                                Integer.parseInt(entry.getEntry().getField(SpecialField.RANKING).get().split("rank")[1]));
                        if (clickedRating.equals(previousRating)) {
                            ranking.setRating(0);
                            event.consume();
                        }
                    }
            } else if (event.getButton().equals(MouseButton.SECONDARY)) {
                event.consume();
            }
        });

Hope that helps you a little bit and maybe you can find a better solution building on this workaround idea :)

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

Successfully merging this pull request may close these issues.

Rank: a single click do not unset the field value
7 participants