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

👌 IMPROVE: don't open first mail by default #6863

Merged
merged 2 commits into from
Aug 4, 2023

Conversation

violoncelloCH
Copy link
Member

fix #6526

This removes the logic to automatically open the first message of a mailbox

In contrary to #2794, this also doesn't open the first mail, when the current folder is reloaded as this would be unexpected: If I open a mailbox, don't select any mail and click on the same mailbox again I don't want the first message to be opened at that time. (Also the current implementation would've lead to first messages being opened while navigation between mailboxes.)

There is one remaining bug: on the initial load of the priority mailbox, this.$store.getters.getEnvelopes(this.mailbox.databaseId, this.searchQuery) returns an empty list even though there are mails in the inbox. Therefore hasEnvelopes is false and the "No message selected" placeholder isn't displayed. After a few seconds (after the third or fourth sync request; but I'm not sure that's actually related) the getEnvelopes getter suddenly returns the correct answer and the view gets updated. I couldn't find out why this getter isn't working at initial load.
Any idea on how to debug this further ?

@violoncelloCH violoncelloCH added enhancement 3. to review papercut Annoying recurring issue with possibly simple fix. labels Jul 10, 2022
@violoncelloCH violoncelloCH self-assigned this Jul 10, 2022
@ChristophWurst
Copy link
Member

Therefore hasEnvelopes is false and the "No message selected" placeholder isn't displayed. After a few seconds (after the third or fourth sync request; but I'm not sure that's actually related) the getEnvelopes getter suddenly returns the correct answer and the view gets updated. I couldn't find out why this getter isn't working at initial load.
Any idea on how to debug this further ?

Sounds like a Vue reactivity bug I've fought before. Not sure how I solved it last time.

@violoncelloCH
Copy link
Member Author

violoncelloCH commented Jul 13, 2022

Yes, indeed...
I did some further debugging and found that getters.getMailbox(mailboxId).envelopeLists initially only contains one entry: "is:pi-other": [ 4, 3 ] only later on a second one is added "": [ 4, 3 ] (that's when getEnvelopes actually returns the list of envelopes instead of just an empty list... no idea though where that second entry comes from and why it's only added at that time and not initially

@violoncelloCH
Copy link
Member Author

Okay, some more results:
The problem seems to be that only the sync (with timeout 30*1000) in App.vue (this exactly matches the delay of 30 seconds I get for the "No message selected" message to appear)

await this.$store.dispatch('syncInboxes')

triggers the async action syncInboxes

mail/src/store/actions.js

Lines 713 to 715 in 916e98a

await dispatch('fetchEnvelopes', {
mailboxId: mailbox.databaseId,
})

which in turn dispatches fetchEnvelopes with query undefined
this then commits addEnvelope with query being empty/undefined:

mail/src/store/actions.js

Lines 467 to 471 in 916e98a

commit('addEnvelope', {
query,
envelope,
addToUnifiedMailboxes,
})

so only at that point, mailbox.envelopeLists actually contains an element with index being an empty string...

My question now is: Is that element of envelopeLists actually supposed to have an empty string as index ?
If yes, how should this be properly fixed ?

  • By doing a this.$store.dispatch('syncInboxes') at the initial loading of the page ?
  • or just by doing dispatch('fetchEnvelopes') with undefined query parameter at initial load ?
  • or is there a better way I don't see yet ?

@ChristophWurst
Copy link
Member

My question now is: Is that element of envelopeLists actually supposed to have an empty string as index ?
If yes, how should this be properly fixed ?

Empty string is the unfiltered envelope list. That would be used when you view your real inbox, not the unified one. The unified inbox consists of three filtered inboxes.

The background sync always uses the unfiltered inbox. So on page load there is no list for ''. Only the first background sync will initialize that.

So that is not a bug and fetching that list on page load is hopefully not necessary.

Therefore hasEnvelopes is false and the "No message selected" placeholder isn't displayed.

It is or is not displayed?

@violoncelloCH
Copy link
Member Author

violoncelloCH commented Jul 14, 2022

Thanks for the explanation !

It is or is not displayed?

On the unified inbox, the placeholder is not displayed initially but only after 30 seconds, once there exists a list for ''.
If I understand it correctly, the hasEnvelope lookup should query for is:pi-other, is:pi-important and is:pi-starred instead of '' in the case of the unified inbox. I'll check how to fix this.

@violoncelloCH
Copy link
Member Author

friendly ping :)
could I get some feedback on this now that I've implemented a fix ^ ?

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. Thanks a lot for your PR.

src/components/MailboxThread.vue Outdated Show resolved Hide resolved
@jancborchardt
Copy link
Member

@violoncelloCH could you add a screenshot for easier design review? :) What would be great is an emptycontent screen like:

mail icon
Welcome to Nextcloud Mail (h2)
Write a new message (as a button)

@GretaD
Copy link
Contributor

GretaD commented Aug 4, 2023

@jancborchardt please see attached a preview of your comment and if i understood you right,

mail icon
Welcome to Nextcloud Mail (h2)
Write a new message (as a button)

Screenshot from 2023-08-04 19-27-19

@GretaD GretaD merged commit 63f56e2 into main Aug 4, 2023
28 checks passed
@GretaD GretaD deleted the dont-open-mail-by-default branch August 4, 2023 18:38
@GretaD
Copy link
Contributor

GretaD commented Aug 4, 2023

Jan, sorry, it was green, and i just click the merge button. If theres still something to do here, i will push a fixup on Monday 🤦

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review enhancement papercut Annoying recurring issue with possibly simple fix.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not open first message by default
5 participants