Skip to content

Implementation of walkthrough infrastructure #13182

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

Merged
merged 58 commits into from
Jul 3, 2025
Merged

Conversation

Yubo-Cao
Copy link
Contributor

@Yubo-Cao Yubo-Cao commented May 29, 2025

Closes N/A

Initial implementation of the walkthrough feature. See this Google Drive video for demonstration: https://drive.google.com/file/d/19sUz1XoSjP0UkuhUvKQE-MZjmVASy2ot/view?usp=sharing

The completion of the walkthrough is also tracked through modifying the preferences class.

This refs #12664

Mandatory checks

  • I own the copyright of the code submitted and I license it under the MIT license
  • Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (if change is visible to the user)
  • 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.

Sorry, something went wrong.

@Siedlerchr
Copy link
Member

So far I like the idea.
Discussion points for Friday would be:

  • How many steps we want
  • Going back one step?
  • content of the steps
  • Additional steps like adding stuff to groups or creating a new entry or library

@koppor
Copy link
Member

koppor commented May 30, 2025

This refs #12664 somehow :)

@JabRef JabRef deleted a comment from jabref-machine May 30, 2025
Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

First half of first iteration through review. More to come on the weekend.

Co-authored-by: Subhramit Basu <subhramit.bb@live.in>
@calixtus calixtus changed the title Add initial implementation of walkthrough Implementation of walkthrough infrastructure Jul 1, 2025
calixtus
calixtus previously approved these changes Jul 2, 2025
subhramit
subhramit previously approved these changes Jul 2, 2025
Copy link
Member

@subhramit subhramit left a comment

Choose a reason for hiding this comment

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

Just some small nits when reading otherwise lgtm

Comment on lines +196 to +202
CompletableFuture<Void> timeoutFuture = CompletableFuture.runAsync(() -> {
try {
Thread.sleep(HANDLER_TIMEOUT_MS);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
}
});
Copy link
Member

Choose a reason for hiding this comment

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

I think this entire class could make use of some more javadoc - I sense high cognitive load on if someone wants to work on this in future. Things like, why we are using a completable future, etc.
But these refinements can be done in future, we can get this in and start focusing on the next steps.

Comment on lines 141 to 144
.filter(menuItem -> Optional.ofNullable(menuItem.getGraphic())
.map(graphic -> graphic.equals(node)
|| Stream.iterate(graphic, Objects::nonNull, Node::getParent)
.anyMatch(cm -> cm.equals(node)))
Copy link
Member

Choose a reason for hiding this comment

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

If I am not wrong on the expansion:

Suggested change
.filter(menuItem -> Optional.ofNullable(menuItem.getGraphic())
.map(graphic -> graphic.equals(node)
|| Stream.iterate(graphic, Objects::nonNull, Node::getParent)
.anyMatch(cm -> cm.equals(node)))
.filter(menuItem -> Optional.ofNullable(menuItem.getGraphic())
.map(graphic -> graphic.equals(node)
|| Stream.iterate(graphic, Objects::nonNull, Node::getParent)
.anyMatch(contextMenu -> contextMenu.equals(node)))

Copy link
Member

Choose a reason for hiding this comment

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

Good catch

@calixtus
Copy link
Member

calixtus commented Jul 3, 2025

@Yubo-Cao please apply suggestions from @subhramit , then we merge.

@Yubo-Cao Yubo-Cao dismissed stale reviews from subhramit and calixtus via 4290a44 July 3, 2025 12:58
Copy link

trag-bot bot commented Jul 3, 2025

@trag-bot didn't find any issues in the code! ✅✨

@calixtus calixtus added this pull request to the merge queue Jul 3, 2025
Merged via the queue into JabRef:main with commit 3cf371c Jul 3, 2025
41 checks passed
event.consume();
beforeNavigate.run();

CompletableFuture<Void> handlerFuture = new CompletableFuture<>();
Copy link
Member

Choose a reason for hiding this comment

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

This variable is never read? -> always handlerFtuure.complete(null) passed. What is the intention?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intention is that we run onNavigate after the original handler has completed, or a timeout has occurred, or a new window has been created, whichever comes first. To allow for such checking, the handler's future is used more as a signal to indicate that the original handler has finished.

/// @param event the event to navigate
static <T extends Event> void navigate(
Runnable beforeNavigate,
EventHandler<? super T> originalHandler,
Copy link
Member

Choose a reason for hiding this comment

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

Annotate with @Nullable (the other ones are non-null, aren't they?)

/// - The handler has timed out after HANDLER_TIMEOUT_MS milliseconds.
/// - A new window has been opened, or an existing window has been closed.
///
/// Those conditions ensure that we will still navigate if original handler is blocking (e.g., showing a dialog,
Copy link
Member

Choose a reason for hiding this comment

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

"These" conditions

@Yubo-Cao Yubo-Cao deleted the walkthrough branch July 4, 2025 18:41
@Yubo-Cao Yubo-Cao restored the walkthrough branch July 4, 2025 18:46
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.

None yet

5 participants