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

prevents select on click when userSelectAction is none #154

Closed
wants to merge 1 commit into from

Conversation

Aierie
Copy link

@Aierie Aierie commented Oct 4, 2024

No description provided.

Comment on lines 195 to +199
const clickSelect = () => {
if (selection.userSelectAction === UserSelectAction.NONE) {
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

That logic is already covered in the parent @annotorious/annotorious package. Even though the selection.userSelection will be invoked, the actual selection state will always remain empty. See this source

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we cannot compare the userSelectAction with the enum value directly, because the userSelectAction can also be an expression: ((a: T) => UserSelectAction)

Copy link
Author

Choose a reason for hiding this comment

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

Yes - what I'm trying to achieve is to control the selection myself, described in #153

Copy link
Author

Choose a reason for hiding this comment

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

I'll work on the userSelectAction expression, thanks! I just realised this - sorry, am pretty new here.

Copy link
Member

Choose a reason for hiding this comment

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

Good point. Yes, indeed the core package already checks this against the current userSelectionAction (value or function).

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 understand correctly, you are trying to implement multi-selection (CTRL-Click?) behavior. That's not supported across the interface yet. Which means there are probably some gotchas involved. Nonetheless: the selection API already supports a list of annotations (rather than just a single selected one) as you saw.

Copy link
Author

@Aierie Aierie Oct 4, 2024

Choose a reason for hiding this comment

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

Yes, I'm trying to do multi-selection behaviour. The selection API does support a list of annotations, but I have to set userSelectAction to NONE if I want to control it myself* from clicks. And when I do that, clicking on any annotation will clear the list of annotations that my program has selected

Copy link
Author

Choose a reason for hiding this comment

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

I guess - my expectation of inertness is that this default clause does nothing. But if there's important reasons for it to clear the array, I'll find a workaround, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Hm, so as far as I can see (tried just now) NONE does already work as designed. If you click an annotation, the annotator does not fire the select event.

I guess for now, it might be easier for you to manage your own selection list, outside of the annotator. An "official" implementation would have to happen in the @annotorious/core package. It's already planned in here but, alas, not yet implemented:
https://github.com/annotorious/annotorious/blob/main/packages/annotorious-core/src/state/Selection.ts#L61-L80

The idea is that the userSelect function would handle things by itself: check for the CTRL modifier key, and then add the annotation to the list rather than replace it in these lines.

The other issue is, of course, the clickAnnotation event which is supposed to fire independently of the selection, for all annotations, regardless of the current useSelectAction setting. But that, unfortunately, isn't implemented yet. (But would also have to be handled in the SelectionHandler.)

Copy link
Member

Choose a reason for hiding this comment

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

I guess - my expectation of inertness is that this default clause does nothing.

I guess that's something that might be worth discussing. In principle, my view is: clicking an inert annotation would be considered the same as clicking on empty space, which is supposed to clear the selection. What's not covered in the current code, of course, is that you might click an inert annotation that overlaps a non-inert one. In this case, I'd expect the non-inert one to get selected (and the inert one to get ignored). The current code, however, would simply clear the selection as you say.

@rsimon
Copy link
Member

rsimon commented Oct 4, 2024

BTW: if your project is something you can share, I'd love to see it. It's still a bit early days for this library. But I'm interested in seeing as many different use cases & getting it improved as best as possible.

@Aierie
Copy link
Author

Aierie commented Oct 4, 2024

Okay, will share once I've cleaned it up enough to use! It is a tool for doing bottom-up qualitative data synthesis that puts a lot of importance on the raw quotes, following Indi Young's method.

I was originally hacking around with RecogitoJS, but realised the successor checked these boxes:

  • Better rendering to avoid awkward UI when applying certain custom styles to annotation spans (for example ::before pseudo-elements)
  • Undo/redo
  • More control over selection

So thanks! This saves me a bunch of work while making the code cleaner. I'll continue using @recogito/text-annotator-js, but will probably store selection state as a property in the annotation for now.

@rsimon
Copy link
Member

rsimon commented Oct 4, 2024

Sounds great, thanks!

@rsimon rsimon closed this Oct 17, 2024
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.

3 participants