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

Autoremove the previous unsaved annotation when a new piece of text is selected #519

Merged
merged 3 commits into from
Mar 31, 2022

Conversation

jgonggrijp
Copy link
Member

@jgonggrijp jgonggrijp commented Mar 4, 2022

@JeltevanBoheemen I'm asking @tijmenbaarda @oktaal to review the code, but the content of this post is also for your information.

I kind of repurposed #311, which can no longer be reproduced, to represent a similar issue I described in #311 (comment). This branch addresses the latter issue. I thought I fixed that bug on the 0.14.0 release branch (#518), but I apparently didn't, probably because I glanced over the issue title in the project board and mistook it for a different issue that I did solve. The bugfix is not terribly urgent, so it can wait until the next release.

In the READ-IT interface, users can create annotations by selecting a piece of text. We implement this by creating a temporary annotation object, adding it to the collection of (otherwise "real") annotations and then opening an editing panel so that the user can tag and optionally identify the selection. If the user saves the changes, the annotation is persisted to the backend. It stays in the collection of annotations in this case. The user can also cancel the annotation, in which case the temporary object is removed from the collection again.

Before the current fix, the temporary object would also stay in the collection if the user selected another piece of text without explicitly saving or canceling the previous selection first. In this way, it was possible to accumulate "ghost annotations". The ghosts would stay visible until the source was reloaded.

The fix employs a feature of the collection which enables it to keep track of which annotation is currently supposed to be in the center of attention. A focus event is fired on the annotation that must be emphasized. This will automatically trigger blur on the annotation that was previously emphasized, if any. If the temporary object blurs without having been saved (isBlank), we know that it must be removed.

@tijmenbaarda The code change is actually very small, because the focus/blur infrastructure that I described above was already in place. I just needed to tap into it in this particular situation. Our frontend code in EDPOP will look somewhat similar, with events loosely connecting different parts of the code (though without TypeScript), so this seemed like a gentle first introduction. It is OK if you cannot follow the bigger picture yet, reviewing just code details line-by-line is enough.

@jgonggrijp jgonggrijp requested a review from tijmenbaarda March 4, 2022 02:03
@jgonggrijp
Copy link
Member Author

@oktaal Could you review this? @tijmenbaarda is on holiday.

@tijmenbaarda, you are still welcome to review this when you return, even if the branch has already been merged.

@jgonggrijp jgonggrijp requested review from oktaal and removed request for tijmenbaarda March 10, 2022 16:08
flat.once('blur', () => {
if (isBlank(annotation)) collection.underlying.remove(annotation);
});
let editPanel: AnnoEditView;
Copy link
Member

@oktaal oktaal Mar 14, 2022

Choose a reason for hiding this comment

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

Kan dit worden teruggeven als een Promise? Het commentaar hieronder scheelt, maar dit komt nu wel heel erg over als een fragiele hack. Bijvoorbeeld zo is denk ik wat duidelijker dat het niet 'meteen' resultaat teruggeeft.

async selectText() {
  /* … */
  let setEditPanel: (view: AnnoEditView) => void;
  let editPanelPromise = new Promise<AnnoEditView>((resolve) =>
    setEditPanel = resolve
  );
  this.once('reopen-edit-annotation', annoView =>
    setEditPanel(this.editAnnotation(annoView, flat)
  );
  // ...
  flat.trigger('focus', flat);
  return await editPanel;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

De grap is dat het wél meteen resultaat teruggeeft, omdat die hele cascade van events synchroon is. Zal ik dat nog toevoegen aan het commentaar? Bijvoorbeeld Through a synchronous cascade of events, ....

Copy link
Member Author

Choose a reason for hiding this comment

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

@oktaal Ik heb het commentaar uitgebreid. Laat me aub nog even weten of het zo duidelijker is.

@jgonggrijp jgonggrijp requested a review from oktaal March 15, 2022 11:22
@jgonggrijp jgonggrijp merged commit b217007 into develop Mar 31, 2022
@jgonggrijp jgonggrijp deleted the feature/autoremove branch March 31, 2022 15:50
@jgonggrijp jgonggrijp added this to the Next release milestone Mar 31, 2022
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