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

Advanced message search #6546

Merged
merged 1 commit into from
Oct 18, 2022
Merged

Conversation

sazanof
Copy link
Collaborator

@sazanof sazanof commented May 25, 2022

Signed-off-by: Mikhail Sazanov m@sazanof.ru

Hello! I would like to discuss the improvement related to the search for emails by sender, recipient, cc, bcc, subject of the letter, as well as by tags.

This is a PR draft, because, ideally, I would also like to search through the body of the letter. I looked at the code and realized that the body of the letter is searched here:

private function getIdsLocally(Account $account, Mailbox $mailbox, SearchQuery $query, ?int $limit): array {

If &filter=my text is passed to the URL request, 2 search tokens will be generated and passed to this method ($query->getTextTokens())

However, there is a problem that I haven't figured out how to solve yet: I get the exception String contains non-ASCII characters.. Tested with Cyrillic characters. Moreover, if you write one word, there is no mistake. I looked at the ASCII characters table and all the characters used in the query are there. This is the first problem and perhaps the most important. Can you advise on this?

Also, in order to correctly search for a match in the topic, I replace the space with "+" so that the $query->addTextToken() method does not work. I can't say that this is a super solution, but that's how it is... But the search occurs clearly by the phrase.

I also didn't understand how to search by imap_label directly on the IMAP server. =(

The whole search uses the database, but I would like to add another switch "Message body"

I placed the search bar in an obvious place. I remember there was a PR nextcloud/contacts#2711

And I read several messages, that this place will be occupied by other buttons. However, it seems to me that the most optimal place here is for searching (in contacts and mail).

If you are interested in PR, I would ask you to check the operation of this functionality and discuss / get tips for further improvement. Thank you!

Peek 2022-05-25 10-44

@sazanof sazanof changed the title Make users to search and filter in messages (contacts,subject,tags) Make users to search and filter messages (contacts,subject,tags) May 25, 2022
@sazanof sazanof force-pushed the enh/messages-filter-feature branch from 7eae2a1 to 58b931c Compare June 1, 2022 13:59
@sazanof
Copy link
Collaborator Author

sazanof commented Jun 2, 2022

image
also added the ability to pre-filter by type "And": unread, important, favorites

@sazanof sazanof force-pushed the enh/messages-filter-feature branch 2 times, most recently from e652003 to d589113 Compare June 3, 2022 05:12
@sazanof sazanof marked this pull request as ready for review June 14, 2022 06:28
@sazanof sazanof changed the title Make users to search and filter messages (contacts,subject,tags) Advanced message search Jun 14, 2022
@JuliaKirschenheuter JuliaKirschenheuter marked this pull request as draft June 20, 2022 12:32
@JuliaKirschenheuter
Copy link
Contributor

JuliaKirschenheuter commented Jun 20, 2022

That is a great feature!
@nimishavijay what do you think design-wise?

@sazanof sazanof force-pushed the enh/messages-filter-feature branch from d589113 to 5d170aa Compare July 25, 2022 09:07
@nimishavijay
Copy link
Member

At first glance it looks really nice :) The placement makes sense to me, and it seems like it would search only within the current mailbox which is open? For eg, if you are in "Sent" then it would search through only "Sent" and not in "Inbox", is that correct?

I am not able to give more detailed feedback as I don't fully understand the language, could you please post screenshots in English so it is easier for me and anyone else to also review? :)

@sazanof
Copy link
Collaborator Author

sazanof commented Jul 25, 2022

@nimishavijay hello!

Yes, the search takes place in the folder that the user has open, with the exception of the folder "Priority" and "All" mailboxes

image
search by "And" conditions. if the "unread" and "important" checkboxes are marked, then as a result we are looking for unread letters marked "important"
image

@nimishavijay
Copy link
Member

nimishavijay commented Jul 25, 2022

Ah thank you so much :)
It looks great! The placement and flow make sense to me, and as far as I understand that space would be occupied by other actions only during multiselect, and I think that's okay :)

Visually also it looks pretty good :) Only minor feedback from my end

  • The icon for filter could be an MDI tune icon
  • Instead of a toggle for the subject could it be a text input field, with a checkbox below that says "No subject" or something like that
  • Additionally: if you entered text in the search field and then click on the filter icon, the subject could be prefilled with the value you entered
  • I missed the "Important, unread, favourites" section in the beginning, possibly because it looks like a heading due to the placement. We could move it down below to the end, with the label "Marked as" and the 3 checkboxes
  • For the "Search" and "Reset" buttons, it would be more in line with NC design if
    • the 2 buttons are justified justified to the right (right now they are at the center of the modal)
    • the primary button (Search) is on the right of the secondary button (Reset)
  • Also I'm thinking we can replace the icon for "Reset" with an MDI replay icon or we could possibly keep the same icon and change the wording to "Clear", not sure about which to go with. Any preferences @jancborchardt?
  • If possible we could also have a "Has attachments" checkbox? Could be pretty useful and Gmail also has one :)

Really nice work! :)

@jancborchardt
Copy link
Member

Great feedback @nimishavijay, nothing to add! :)

Also I'm thinking we can replace the icon for "Reset" with an MDI replay icon or we could possibly keep the same icon and change the wording to "Clear", not sure about which to go with. Any preferences @jancborchardt?

Agree with your proposal of changing the wording to "Clear".

@sazanof
Copy link
Collaborator Author

sazanof commented Jul 26, 2022

@nimishavijay @jancborchardt HELLO!!!)

I removed the "Theme" switch altogether. it was initially present, as I was thinking of adding a "Body" switch in the future. So that you can choose why to search in the query:
Subject - off
Body - on

But since it's not possible to search through the body now, what's the point of one switch? Therefore, I think that by default, what was written in the search bar is searched in the Subject

please give feedback about corrected design =)

image
image

PS I will also try to make a filter by search date (sentdate)

@sazanof
Copy link
Collaborator Author

sazanof commented Jul 28, 2022

Hello everyone I will be glad if you take a look!
Peek 2022-07-28 11-34

@nimishavijay
Copy link
Member

Nice work! Super cool with the datetime picker :D

A few tiny changes:

  • "Date interval" --> "Date"
  • For the date time picker: How about instead of one field there are 2 separate fields for "from" and "to" dates on the same line? This way only one datetime picker is shown at a time so it's less busy :)
  • "Search" button on the right of "Clear" button
  • Clear button actually does look better with your original MDI close icon so we can so with that :)
  • The entire "Marked as" row can be moved to the bottom, and we can also move "Has attachments" to the next row

Therefore, I think that by default, what was written in the search bar is searched in the Subject

This behaviour makes sense, I think we can also include the subject input field in the modal to easily make changes to the subject if needed. It can be the first item.

Really really nice work :)

@sazanof
Copy link
Collaborator Author

sazanof commented Jul 28, 2022

@nimishavijay
Thanks for the feedback! Take a look, give comments, please =)
image

@sazanof sazanof force-pushed the enh/messages-filter-feature branch from 403a6ee to 85bf7b0 Compare July 28, 2022 20:09
@nimishavijay
Copy link
Member

nimishavijay commented Aug 1, 2022

Looks great now! Only a couple of tiny changes:

  • "Text" --> "Subject"
  • We can remove the trailing ellipses ("...") from the placeholder text in the fields
  • Swap the positions of "Search" and "Clear" buttons
  • Many other clients don't have a time picker in their search so I think we can get rid of it
  • "Pick a date and time" --> "Pick a start date" and "Pick an end date"

sazanof pushed a commit to sazanof/nextcloud_mail that referenced this pull request Aug 1, 2022
Signed-off-by: Mikhail Sazanov <m@sazanof.ru>
@sazanof sazanof force-pushed the enh/messages-filter-feature branch from 85bf7b0 to 6430c1b Compare August 1, 2022 18:50
@sazanof
Copy link
Collaborator Author

sazanof commented Aug 1, 2022

Hello!
@nimishavijay
"Text" --> "Subject" - DONE
We can remove the trailing ellipses ("...") from the placeholder text in the fields - DONE
Swap the positions of "Search" and "Clear" buttons (they always jump over each other) - DONE
Many other clients don't have a time picker in their search so I think we can get rid of it - DONE
"Pick a date and time" --> "Pick a start date" and "Pick an end date" ......... I think it is not possible to change placeholder text without make changes in component
https://github.com/nextcloud/nextcloud-vue/blob/master/src/components/DatetimePicker/DatetimePicker.vue#L305-L322

image

image

What else?

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Very very nice job @sazanof! :) Only minimal feedback:

  • The modal needs a title (h2), inside the modal "Search in mailbox"
  • Can the "x" close button also be placed inside the modal on the top right, or is that a component thing cc @GretaD ?
  • The placeholder of "Subject" can then be changed to "Search term"
  • The date placeholders would be more understandable as "Pick start date" and "Pick end date"
  • The "Select recipient" placeholder could be individual to the field, like:
    • From: "Select sender"
    • To: "Select recipient"
    • Cc: "Select cc recipient"
    • Bcc: "Select bcc recipient"
  • On the bottom "Favorite" is better as singular – "Marked as favorite"

Otherwise all good to go! :)

@sazanof
Copy link
Collaborator Author

sazanof commented Aug 25, 2022

Hi, @jancborchardt ! Thank you for review!
Surprize after updates =)
Now it remains to change the header a little and push the search here =)
image
I'm thinking of limiting myself to the search icon or showing the panel by clicking "search" icon.
Do you have any ideas?

The date placeholders would be more understandable as "Pick start date" and "Pick end date"

I think it is not possible to change placeholder text without make changes in component
https://github.com/nextcloud/nextcloud-vue/blob/master/src/components/NcDatetimePicker/NcDatetimePicker.vue#L310-L327

@szaimen szaimen requested a review from st3iny October 11, 2022 09:20
@st3iny
Copy link
Member

st3iny commented Oct 11, 2022

Help! Well, after update code to NcComponentTitle.vue from ComponentTitle.vue there are some visual inconsistencies with components image Will this be corrected in the nexctoud-vue repository, or should I fix it here?

Don't worry, this is already fixed in nextcloud-vue.

Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Tested and works.

The whole search uses the database, but I would like to add another switch "Message body"

That is a great idea! However, I'm currently not sure either how to implement it. IMO, we should move on with this PR and followup with improvements later.

Comment on lines +10 to +18
position: absolute;
top: 0;
bottom: 0;
left: 0;
right: 0;
display: flex;
flex-direction: column;
align-items: center;
justify-content: center;
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity: Are those changes required for the PR? They seem to be unrelated.

Copy link
Collaborator Author

@sazanof sazanof Oct 11, 2022

Choose a reason for hiding this comment

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

That's right! I just wanted to vertically center the text "Loading messages"

@sazanof sazanof force-pushed the enh/messages-filter-feature branch 2 times, most recently from 5ca3a1a to cd8054f Compare October 15, 2022 10:54
@sazanof
Copy link
Collaborator Author

sazanof commented Oct 15, 2022

So, conflicts solved, css fixrd a little

@st3iny
Copy link
Member

st3iny commented Oct 18, 2022

I'm not sure why the php tests fail. I'll investigate it a bit.

In the meantime, could you please fix all static analysis issues in your php code changes?
https://github.com/nextcloud/mail/actions/runs/3255416357/jobs/5344711652

@st3iny st3iny marked this pull request as ready for review October 18, 2022 12:40
Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Tested and works! I love it!

Again, a big thank you from me and the rest of the Groupware team.

I added a couple of commits to fix styling and removed some dead debugging code. Could you please squash all commits (including my fixups) into one? Then we are ready to merge.

Signed-off-by: Mikhail Sazanov <m@sazanof.ru>
@sazanof
Copy link
Collaborator Author

sazanof commented Oct 18, 2022

@st3iny Thank you for help.

into one

I hope I did everything right

@st3iny st3iny dismissed jancborchardt’s stale review October 18, 2022 12:59

All points were addressed.

@st3iny st3iny merged commit f264e71 into nextcloud:main Oct 18, 2022
@miaulalala
Copy link
Contributor

Thanks so much for your work, it is impressive!!!!! 🥳

@sazanof sazanof deleted the enh/messages-filter-feature branch October 18, 2022 15:42
@szaimen
Copy link
Contributor

szaimen commented Oct 20, 2022

🎉🎉🎉🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants