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

Modify search rules for find by title keyphrase #265

Merged

Conversation

zhaojj2209
Copy link

Resolves #242.

Find by title now matches by keyphrase instead of keyword, and allows partial word matches.

Also included are minor edits to the user guide.

@zhaojj2209 zhaojj2209 added type.bug 🐛 Something isn't working type.documentation 🧻 Improvements or additions to documentation priority.high 🥇 Overdue changes; fixing this is critical labels Oct 31, 2020
@zhaojj2209 zhaojj2209 added this to the v1.4 milestone Oct 31, 2020
@zhaojj2209 zhaojj2209 requested a review from a team October 31, 2020 18:44
@codecov-io
Copy link

codecov-io commented Oct 31, 2020

Codecov Report

Merging #265 into master will decrease coverage by 0.50%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #265      +/-   ##
============================================
- Coverage     71.64%   71.14%   -0.51%     
- Complexity      900      903       +3     
============================================
  Files           141      141              
  Lines          2825     2852      +27     
  Branches        331      336       +5     
============================================
+ Hits           2024     2029       +5     
- Misses          682      704      +22     
  Partials        119      119              
Impacted Files Coverage Δ Complexity Δ
...2103_w16_3/finesse/logic/commands/FindCommand.java 100.00% <ø> (ø) 8.00 <0.00> (ø)
..._cs2103_w16_3/finesse/commons/util/StringUtil.java 93.33% <100.00%> (-1.41%) 6.00 <0.00> (-1.00)
...ion/predicates/TitleContainsKeywordsPredicate.java 100.00% <100.00%> (ø) 7.00 <1.00> (ø)
...c/commands/bookmark/EditBookmarkIncomeCommand.java 87.87% <0.00%> (-5.46%) 10.00% <0.00%> (ø%)
.../commands/bookmark/EditBookmarkExpenseCommand.java 87.87% <0.00%> (-5.46%) 10.00% <0.00%> (ø%)
...021s1_cs2103_w16_3/finesse/ui/TransactionCard.java 0.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
...3/finesse/ui/bookmark/BookmarkTransactionCard.java 0.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
...okmarkparsers/EditBookmarkIncomeCommandParser.java 93.33% <0.00%> (+0.22%) 11.00% <0.00%> (ø%)
...kmarkparsers/EditBookmarkExpenseCommandParser.java 93.33% <0.00%> (+0.22%) 11.00% <0.00%> (ø%)
...6_3/finesse/logic/parser/FinanceTrackerParser.java 72.58% <0.00%> (+0.44%) 31.00% <0.00%> (ø%)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e3a4e9...92aed60. Read the comment docs.

docs/UserGuide.md Outdated Show resolved Hide resolved
docs/UserGuide.md Outdated Show resolved Hide resolved
docs/UserGuide.md Outdated Show resolved Hide resolved
docs/UserGuide.md Outdated Show resolved Hide resolved
docs/UserGuide.md Show resolved Hide resolved
docs/UserGuide.md Outdated Show resolved Hide resolved
docs/UserGuide.md Show resolved Hide resolved
Copy link
Member

@ianyong ianyong left a comment

Choose a reason for hiding this comment

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

When searching using an empty title find t/ , the following is printed in the console:

Exception in thread "JavaFX Application Thread" java.lang.RuntimeException: java.lang.reflect.InvocationTargetException
        at javafx.fxml.FXMLLoader$MethodHandler.invoke(FXMLLoader.java:1787)
        at javafx.fxml.FXMLLoader$ControllerMethodEventHandler.handle(FXMLLoader.java:1670)
        at com.sun.javafx.event.CompositeEventHandler.dispatchBubblingEvent(CompositeEventHandler.java:86)
        at com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:238)
        at com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:191)
        at com.sun.javafx.event.CompositeEventDispatcher.dispatchBubblingEvent(CompositeEventDispatcher.java:59)
        at com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:58)
        at com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
        at com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
        at com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
        at com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
        at com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
        at com.sun.javafx.event.EventUtil.fireEventImpl(EventUtil.java:74)
        at com.sun.javafx.event.EventUtil.fireEvent(EventUtil.java:49)
        at javafx.event.Event.fireEvent(Event.java:198)
        at javafx.scene.Node.fireEvent(Node.java:8879)
        at com.sun.javafx.scene.control.behavior.TextFieldBehavior.fire(TextFieldBehavior.java:184)
        at com.sun.javafx.scene.control.behavior.TextInputControlBehavior.lambda$keyMapping$62(TextInputControlBehavior.java:330)
        at com.sun.javafx.scene.control.inputmap.InputMap.handle(InputMap.java:274)
        at com.sun.javafx.event.CompositeEventHandler$NormalEventHandlerRecord.handleBubblingEvent(CompositeEventHandler.java:218)
        at com.sun.javafx.event.CompositeEventHandler.dispatchBubblingEvent(CompositeEventHandler.java:80)
        at com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:238)
        at com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:191)
        at com.sun.javafx.event.CompositeEventDispatcher.dispatchBubblingEvent(CompositeEventDispatcher.java:59)
        at com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:58)
        at com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
        at com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
        at com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
        at com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
        at com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
        at com.sun.javafx.event.EventUtil.fireEventImpl(EventUtil.java:74)
        at com.sun.javafx.event.EventUtil.fireEvent(EventUtil.java:54)
        at javafx.event.Event.fireEvent(Event.java:198)
        at javafx.scene.Scene$KeyHandler.process(Scene.java:4058)
        at javafx.scene.Scene$KeyHandler.access$1500(Scene.java:4004)
        at javafx.scene.Scene.processKeyEvent(Scene.java:2121)
        at javafx.scene.Scene$ScenePeerListener.keyEvent(Scene.java:2595)
        at com.sun.javafx.tk.quantum.GlassViewEventHandler$KeyEventNotification.run(GlassViewEventHandler.java:217)
        at com.sun.javafx.tk.quantum.GlassViewEventHandler$KeyEventNotification.run(GlassViewEventHandler.java:149)
        at java.base/java.security.AccessController.doPrivileged(Native Method)
        at com.sun.javafx.tk.quantum.GlassViewEventHandler.lambda$handleKeyEvent$1(GlassViewEventHandler.java:248)
        at com.sun.javafx.tk.quantum.QuantumToolkit.runWithoutRenderLock(QuantumToolkit.java:390)
        at com.sun.javafx.tk.quantum.GlassViewEventHandler.handleKeyEvent(GlassViewEventHandler.java:247)
        at com.sun.glass.ui.View.handleKeyEvent(View.java:547)
        at com.sun.glass.ui.View.notifyKey(View.java:971)
        at com.sun.glass.ui.win.WinApplication._runLoop(Native Method)
        at com.sun.glass.ui.win.WinApplication.lambda$runLoop$3(WinApplication.java:174)
        at java.base/java.lang.Thread.run(Thread.java:834)
Caused by: java.lang.reflect.InvocationTargetException
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:566)
        at com.sun.javafx.reflect.Trampoline.invoke(MethodUtil.java:76)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:566)
        at com.sun.javafx.reflect.MethodUtil.invoke(MethodUtil.java:273)
        at com.sun.javafx.fxml.MethodHelper.invoke(MethodHelper.java:83)
        at javafx.fxml.FXMLLoader$MethodHandler.invoke(FXMLLoader.java:1784)
        ... 47 more
Caused by: java.lang.IllegalArgumentException: Word parameter cannot be empty
        at ay2021s1_cs2103_w16_3.finesse.commons.util.AppUtil.checkArgument(AppUtil.java:39)
        at ay2021s1_cs2103_w16_3.finesse.commons.util.StringUtil.containsIgnoreCase(StringUtil.java:30)
        at ay2021s1_cs2103_w16_3.finesse.model.transaction.predicates.TitleContainsKeywordsPredicate.lambda$test$0(TitleContainsKeywordsPredicate.java:22)
        at java.base/java.util.stream.MatchOps$1MatchSink.accept(MatchOps.java:90)
        at java.base/java.util.ArrayList$ArrayListSpliterator.tryAdvance(ArrayList.java:1631)
        at java.base/java.util.stream.ReferencePipeline.forEachWithCancel(ReferencePipeline.java:127)
        at java.base/java.util.stream.AbstractPipeline.copyIntoWithCancel(AbstractPipeline.java:502)
        at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:488)
        at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
        at java.base/java.util.stream.MatchOps$MatchOp.evaluateSequential(MatchOps.java:230)
        at java.base/java.util.stream.MatchOps$MatchOp.evaluateSequential(MatchOps.java:196)
        at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
        at java.base/java.util.stream.ReferencePipeline.anyMatch(ReferencePipeline.java:528)
        at ay2021s1_cs2103_w16_3.finesse.model.transaction.predicates.TitleContainsKeywordsPredicate.test(TitleContainsKeywordsPredicate.java:22)
        at ay2021s1_cs2103_w16_3.finesse.model.transaction.predicates.TitleContainsKeywordsPredicate.test(TitleContainsKeywordsPredicate.java:12)
        at java.base/java.util.function.Predicate.lambda$and$0(Predicate.java:69)
        at javafx.collections.transformation.FilteredList.refilter(FilteredList.java:332)
        at javafx.collections.transformation.FilteredList.access$000(FilteredList.java:50)
        at javafx.collections.transformation.FilteredList$1.invalidated(FilteredList.java:102)
        at javafx.beans.property.ObjectPropertyBase.markInvalid(ObjectPropertyBase.java:112)
        at javafx.beans.property.ObjectPropertyBase.set(ObjectPropertyBase.java:147)
        at javafx.collections.transformation.FilteredList.setPredicate(FilteredList.java:125)
        at ay2021s1_cs2103_w16_3.finesse.model.ModelManager.updateFilteredTransactionList(ModelManager.java:264)
        at ay2021s1_cs2103_w16_3.finesse.model.ModelManager.updateFilteredTransactionList(ModelManager.java:272)
        at ay2021s1_cs2103_w16_3.finesse.logic.commands.FindTransactionCommand.execute(FindTransactionCommand.java:22)
        at ay2021s1_cs2103_w16_3.finesse.logic.LogicManager.execute(LogicManager.java:52)
        at ay2021s1_cs2103_w16_3.finesse.ui.MainWindow.executeCommand(MainWindow.java:245)
        at ay2021s1_cs2103_w16_3.finesse.ui.CommandBox.handleCommandEntered(CommandBox.java:98)
        ... 59 more

ianyong
ianyong previously approved these changes Nov 3, 2020
Copy link
Member

@ianyong ianyong left a comment

Choose a reason for hiding this comment

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

LGTM!

@Override
public boolean test(Transaction transaction) {
return keyphrases.stream()
.anyMatch(keyword -> StringUtil.containsIgnoreCase(transaction.getTitle().toString(), keyword));

Choose a reason for hiding this comment

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

Suggested change
.anyMatch(keyword -> StringUtil.containsIgnoreCase(transaction.getTitle().toString(), keyword));
.anyMatch(keyphrase -> StringUtil.containsIgnoreCase(transaction.getTitle().toString(), keyphrase));

Copy link

@yongping827 yongping827 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@ianyong ianyong left a comment

Choose a reason for hiding this comment

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

LGTM!

@ianyong ianyong merged commit 0adb55c into AY2021S1-CS2103T-W16-3:master Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority.high 🥇 Overdue changes; fixing this is critical type.bug 🐛 Something isn't working type.documentation 🧻 Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[PE-D] Unhandled exception when using find with a keyword that contains whitespace
4 participants