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

Technical Documentation and Prototype #2

Open
wants to merge 36 commits into
base: develop
Choose a base branch
from
Open

Conversation

josphstar
Copy link
Owner

@josphstar josphstar commented Nov 14, 2021

The following things have been added:

  • Updated function specification document
  • Technical Documentation
  • Merged existing refine-search branch
  • Rudimentary prototype for better search implementation
    • Dropdown menu
    • Clickable buttons for attributes and logical operations
    • Rudimentary button logic
    • Testing recent search list

The prototype allowed us to recognize early mistakes (not isolating functions enough for example) and adjust with some additional classes like i.e. SearchFieldSynchronizer, which will be further improved upon during the upcoming days.

PS: This repo josphstar/jabref is a new, clean fork from JabRef/jabref and will be our working repo from now on. For prior pull requests visit the old repo josphstar/jabref2.

koppor and others added 18 commits November 1, 2021 16:34
Add Lucene functionality to SearchBar
- Fixed odd behaviour where dropdown reapppears when repeatedly clicking search bar
- Removed detachable attrubute (for now?)
- Corrected Checkstyle
- Added test method to author button (dont allow attributes without search terms)
- Fixed odd behaviour where dropdown reapppears when repeatedly clicking search bar
- Removed detachable attrubute (for now?)
- Corrected Checkstyle
- Buttons dont allow adding attributes when prior ones are empty
Corrected some minor formatting errors
@colinhex colinhex self-requested a review November 15, 2021 01:13
Copy link
Collaborator

@marcelluethi marcelluethi left a comment

Choose a reason for hiding this comment

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

Sehr gute Arbeit. Wir haben einige kleine Kommentare eingefügt und werden am Mittwoch noch einige Dinge besprechen.

docs/sweng/technical-documentation.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@colinhex colinhex left a comment

Choose a reason for hiding this comment

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

Good work on the prototype already!

josphstar and others added 2 commits November 16, 2021 15:51
Also modified RecentSearch such that empty strings do not get added to
the list.
Copy link
Collaborator

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Minor comments to the reuqiremnts specification. In general: looks good 👍

docs/sweng/pflichtenheft.md Outdated Show resolved Hide resolved
docs/sweng/pflichtenheft.md Outdated Show resolved Hide resolved
docs/sweng/pflichtenheft.md Outdated Show resolved Hide resolved
docs/sweng/pflichtenheft.md Outdated Show resolved Hide resolved
docs/sweng/pflichtenheft.md Outdated Show resolved Hide resolved
docs/sweng/pflichtenheft.md Outdated Show resolved Hide resolved
docs/sweng/pflichtenheft.md Outdated Show resolved Hide resolved
* Preconditions: JabRef must be started
* Procedure:
* Click on the global search bar.
* Choose the fields you would like to search in (i.e. author, title etc.).
Copy link
Collaborator

Choose a reason for hiding this comment

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

The proposed fields should be sorted as follows:

  • used in the current library (this is more a filtering, see above)
  • first the required fields then the optional fields; alphabetically (this is more implementation effort, because information of all entry types have to be merged. In case a field is required.in one, it is treated as required)
  • one could also be more smart to sort by usage in the library (or by usage in recent searches). --> much more effort

For me, a hard coded order is also OK. Title, Author, groups, keywords - then all others

(This should be moved to the features)

Copy link
Owner Author

@josphstar josphstar Nov 21, 2021

Choose a reason for hiding this comment

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

We don't understand how the distinction between required and optional is made in relation to our UI buttons. I think we are missing something?

Currently we have a prototype with fixed buttons for major fields (author, title, journal, year, [year-range]), as seen in #341. Those are always showing in the dropdown menu regardless of input.

docs/sweng/pflichtenheft.md Outdated Show resolved Hide resolved
docs/sweng/technical-documentation.md Outdated Show resolved Hide resolved
@josphstar
Copy link
Owner Author

josphstar commented Nov 21, 2021

@koppor Thanks for the concise feedback! I'm very sorry to say that one of us mistakenly committed an outdated, early draft of our functional specification document which you then reviewed. Nevertheless we got a lot of good points from it which also apply to the actual document. functional-specification-document.md is now corrected.

@josphstar
Copy link
Owner Author

josphstar commented Nov 21, 2021

We have some questions we would like to clear up:

  • Complex search query: As far as we understood Lucene is used to parse the text field content from the search field, but the query is still passed on to JabRef's internal complex search query. Our implementation must ensure that interaction with the GUI follows Lucene syntax. I understand JabRef wants to get rid of its own complex search and use Lucene entirely in the end. Is the internal overhaul of the search within our capabilities for the semester and also demanded for our issue?
  • The way we have implemented our UI prototype so far is according to #341 with the PopOver class. @koppor you have mentioned JabRef wants to move away from popups, is such an implementation still up-to-date?
  • We dont understand what required and optional fields are in the context of our UI buttons for the fields, see comment above.
  • When not providing a logical operation OR/AND, should the default be dynamic or only OR, as described in this comment?

    If the query is not a Lucene query, just take each keyword and do an OR search.

    I would argue that an AND search would be more appropriate, except if one field appears twice. So:

    author=koppor author=btut -> OR search author=koppor title=JabRef -> AND search

Thanks for your efforts ^^

@colinhex
Copy link
Collaborator

We have some questions we would like to clear up:

  • Complex search query: As far as we understood Lucene is used to parse the text field content from the search field, but the query is still passed on to JabRef's internal complex search query. Our implementation must ensure that interaction with the GUI follows Lucene syntax. I understand JabRef wants to get rid of its own complex search and use Lucene entirely in the end. Is the internal overhaul of the search within our capabilities for the semester and also demanded for our issue?
  • The way we have implemented our UI prototype so far is according to #341 with the PopOver class. You have mentioned JabRef wants to move away from popups, is such an implementation still up-to-date?
  • We dont understand what required and optional fields are in the context of our UI buttons for the fields, see comment above.
  • When not providing a logical operation OR/AND, should the default be dynamic or only OR, as described in this comment?

    If the query is not a Lucene query, just take each keyword and do an OR search.

    I would argue that an AND search would be more appropriate, except if one field appears twice. So:
    author=koppor author=btut -> OR search author=koppor title=JabRef -> AND search

Thanks for your efforts ^^

Just as to what is in your capabilities, it will be up to you to estimate. If there is an overhaul planned that you think exceeds your limits timewise I suggest you for sure implement what you wanted to implement anyway so that it keeps the future changes in mind/doesn't overcomplicate them. After the project concludes you have the option to continue working with JabRef and finish it off.

@koppor
Copy link
Collaborator

koppor commented Nov 23, 2021

* Complex search query: As far as we understood Lucene is used to parse the text field content from the search field, 

Yes. Therefore, I would suggest that your pull request targe refine-search, which parses using the Lucene syntax. (Pull Request JabRef#8206)

That means: The main branch is not updated with this regard. Sorry for the confusion here.

but the query is still passed on to JabRef's internal complex search query.

No. I re-implemented the search based on Lucene.

Our implementation must ensure that interaction with the GUI follows Lucene syntax.

Yes.

I understand JabRef wants to get rid of its own complex search and use Lucene entirely in the end. Is the internal overhaul of the search within our capabilities for the semester and also demanded for our issue?

The internal search is already overhouled. See JabRef#8206.

You do not need to implement a search for "First Author", "Last Author", "Affiliation". Just use the fields available from JabRef. (In case there is time left, you could work on that, but this is hard stuff. Hint Line 73 in BibQueryVisitor: if (searchField.isEmpty() || searchField.equalsIgnoreCase("anyfield")) {)

* The way we have implemented our UI prototype so far is according to [#341](https://github.com/koppor/jabref/issues/341) with the [PopOver class](http://javadox.com/org.controlsfx/controlsfx/8.0.5/org/controlsfx/control/PopOver.html). @koppor  you have mentioned JabRef wants to move away from popups, is such an implementation still up-to-date?

We need to distingish PopOvers and PopOuts. -- We need to be clear in teams. See also https://ux.stackexchange.com/q/90336/93436. -- Maybe you should add these two terms to the glossary?

  • PopOver: https://en.wikipedia.org/wiki/Popover_(GUI)
  • PopUp: "Popup - These are the assholes of UI Design in this category. They say "Hey, I'm here now. You don't have to deal with me right now, keep doing what you're doing, but I'm not going away until you say you don't want me around anymore." Basically an action you take as a user means something new is shown and you've got to deal with it at some point. They are especially painful if they play noise." -- https://www.quora.com/Whats-the-difference-between-a-modal-a-popover-and-a-popup -- In other words: A popup is a modal window which asks for confirmation.

Since Popups "are the assholes of UI design", JabRef wants to move away from them.

PopOvers are IMHO placed nicely in the UI: They appear right at the place where they are needed.

See the "import from an ID" for a good PopOver example:

grafik

* We dont understand what required and optional fields are in the context of our UI buttons for the fields, see [comment above](https://github.com/josphstar/jabref/pull/2#discussion_r753650629).

This is an advanced feature, you do not need to implement. "For me, a hard coded order is also OK." (which you pointed out by linking to the issue)


For completeness I can reiterate one of my comment

  • used in the current library (this is more a filtering, see above)

One can get the used required fields similar to that:

frame.getCurrentLibraryTab().getDatabase().getEntries().stream()
                     .map(entry -> entry.getType())
                        .distinct()
                        .flatMap(type -> BibtexEntryTypeDefinitions.ALL.stream().filter(t -> t.getType().equals(type)))
                        .sorted()

That would change "Title" "Summary" "Author" "First Author" in the UI element

grafik


* When not providing a logical operation OR/AND, should the default be dynamic or only OR, as described in [this comment](https://github.com/koppor/jabref/issues/341#issuecomment-966264846)?
  > > If the query is not a Lucene query, just take each keyword and do an OR search.

The idea was to be like google, which doesn't do a strict AND. (in the context of the web search we already have the default OR in place)

@btut any input here? What is the thing we do in the PDF full text search with Lucene backend?

@koppor
Copy link
Collaborator

koppor commented Nov 23, 2021

Regarding AND and OR, @btut already made a statement at JabRef#341 (comment). I think, there should be a constant which can be changed easily when we are sure how to handle.

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.

8 participants