All: gsoc project - Add search filters #3213
Conversation
naviji
changed the title
|
Refer https://discourse.joplinapp.org/t/database-speed-test-results/8766 for reasons for changing notes_fts virtual table and the SQL search query. |
|
Thanks for the update @naviji. I see the pull request is still with draft status, does it mean you are still planning to do more work on it? Also, as this is a complex pull request, it would help if you document how it currently works:
Your code seems to be quite modular and well organised, which is great, but additional doc like this would help approach the pull request. It can be UML diagrams, Markdown or whatever format you prefer to use as long as it's clear. |
|
Also we need to think how we can release this. The issue is that it's not a feature we can put behind a beta flag like the Code MIrror editor, as there are major database changes and it affects desktop, mobile and cli apps. So what I think is that we'll do a pre-release with it but we'll make sure that the implementation is very stable (nearly production ready), so we'll rely on your test units and some extra manual testing. By the way, did you test on mobile and cli so far? |
|
Documentation for PR: https://discourse.joplinapp.org/t/documentation-for-the-search-filter-pr/9770 |
|
Please let me know when it's ready to review. |
|
I'll mention this in the docs. |
ReactNativeClient/lib/time-utils.js
Outdated
| goBackInTime(n, timeDuration) { | ||
| // Note that we are starting from the first ms of the current timeDuration | ||
| // eg. If we go back by one day we are subtracting (24*60*60*1000) ms from the start ms of today | ||
| return moment().startOf(timeDuration).subtract(n, timeDuration).format('x'); | ||
| } | ||
|
|
||
| goForwardInTime(n, timeDuration) { | ||
| return moment().startOf(timeDuration).add(n, timeDuration).format('x'); | ||
| } |
There was a problem hiding this comment.
As a first argument to these function, please pass the date that should go forward/backward. Also please clarify what is "n" (possible values, unit) and what is timeDuration (possible values, unit, as from your code it seems to be "day", "hours", etc. but from your example it seems to be milliseconds).
There was a problem hiding this comment.
I didn't find tests for these two functions. Please add some if you haven't already done so.
There was a problem hiding this comment.
OK. I modified the function and added some clarification.
Where should I place the tests? I can't find any existing tests for timeUtils functions.
Also, do we need a unit test here as we're just calling moment.js functions as per its doc? I haven't written any additional logic.
There was a problem hiding this comment.
In that case please create a new file for it. For example duplicate tests/ArrayUtils.js and rename to tests/timeUtils.js. Even though it's relatively straightforward implementation of the moment.js api, it's still good to have a test units to specify what cases exactly we support. It would also be useful if for whatever reason we change from moment.js to another time library.
|
@naviji, there's a conflict on the migrations due to some issue with a recent pull request that had to be reverted. I think you just need to adjust the migration number to fix the conflict. |
|
By the way, did you have the opportunity to test the feature on mobile and CLI? |
|
Yes, I did test on mobile and cli. |
I ported all the existing filters into the new implementation.
I also added a new filter on tags.
tag: docs tag:gsoc