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.

Sign up for GitHub

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

Jump to bottom

Show Notes Loading not No Notes when notes are loading #1649

Closed
wants to merge 2 commits into from
Closed

Show Notes Loading not No Notes when notes are loading #1649

wants to merge 2 commits into from

Conversation

belcherj
Copy link
Contributor

Fix

When notes are loading we should not display the text "No Notes" as there may be notes but they have yet to load. This PR makes the text "Notes Loading" until notes are loaded and then changes to "No Notes" if there are indeed no notes.

Test

  1. Have lots of notes and throttle your connection, load the app
  2. Observe the text in the notes list no says "Notes Loading"
  3. Have an account with no notes
  4. Observe the text says "No Notes" once notes have loaded

Review

Only one developer id required to review these changes, but anyone can perform the review.

Release

RELEASE-NOTES.txt was updated with:

  • Display a notice that notes are loading when notes are loading

@belcherj belcherj self-assigned this Oct 18, 2019
@belcherj belcherj added this to the 1.10 milestone Oct 18, 2019
@belcherj belcherj requested a review from a team October 18, 2019 15:43
dmsnell
dmsnell reviewed Oct 18, 2019
View reviewed changes
lib/flux/app-state.js
@@ -39,6 +39,7 @@ export const actionMap = new ActionMap({
revision: null,
showTrash: false,
listTitle: 'All Notes',
notesLoaded: false,
Copy link
Member

Choose a reason for hiding this comment

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

Did we consider starting notes: null?

There's a chance to indicate whether or not we've loaded using this tri-state value and that could preclude the need for creating a second value intrinsically bound to the same data.

const noteListStatus = notes =>
  null === notes ? 'Loading…' :
  0 === notes.length ? 'No Notes' :
  'Has notes'

Copy link
Member

Choose a reason for hiding this comment

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

Just an explanation: I usually try to find ways of gleaning more information from a value without adding additional flags or meta as they tend to get out of sync and because people tend to zoom-in on the value they think they need as soon as they find a value that seems to work, so for dev communication purposes I have found the tai-state value quite useful when there is a separate unknown or pending state from the empty state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making the default value for notes null causes some plumbing issues. Setting the default to null doesn't do much. It immediately gets overwritten to an array. Changing the way this works I think is outside the scope of this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A month or so ago I spent several hours trying to get app state to function as you describe and I created more bugs than I was solving.

Copy link
Member

Choose a reason for hiding this comment

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

ah interesting. I wondered what would break if we made the change.

where does it get immediately overwritten? on authChanged I can see we'd have to set it null there too.

I wouldn't question this further other than I recognize that we're adding new stuff into the app and doing so with two variables that are storing some of the same data, hypothetically.

I'll do some quick checks in a running copy locally and then report back.

Nice work, and I did at one point see your old state PR. state here in general is in a place it shouldn't be. long-term we can trim it way down

dmsnell added a commit that referenced this pull request Oct 18, 2019
@dmsnell
Note list: Distinguish loading from empty states
bca3e54 
Alternative idea to #1649

There are a few times when we boot the app and our list of notes is
empty because we haven't received udpates from the server yet. This
happens on intial app boot, immediately after authorizing, and when
we lose our local copy of the notes from `IndexedDB`.

During these times we're showing that there are no notes in the account
which is misleading. In this patch we're starting with a `null` value
for the notes so that we can distinguish between "there are no notes
in the account" and "we haven yet to determine which notes are in the
account."

In comparison to #1649 we're using a tri-state value for `notes` instead
of introducing an additional flag which must be kept in sync with
`notes`.
@dmsnell dmsnell mentioned this pull request Oct 18, 2019
Merged
@dmsnell
Copy link
Member

dmsnell commented Oct 18, 2019

@belcherj I created #1650 to tease out the ideas in my comment. I had to update filterNotes() but beyond that I don't think there was anything else that was broken.

in this PR one thing we need to do is update authChanged in app-state so that we reset the flag otherwise we'll get No Notes again while waiting to receive the first index from Simperium.

belcherj pushed a commit that referenced this pull request Oct 21, 2019
@dmsnell @belcherj
Note list: Distinguish loading from empty states (#1650)
e54b2ae 
Alternative idea to #1649

There are a few times when we boot the app and our list of notes is
empty because we haven't received udpates from the server yet. This
happens on intial app boot, immediately after authorizing, and when
we lose our local copy of the notes from `IndexedDB`.

During these times we're showing that there are no notes in the account
which is misleading. In this patch we're starting with a `null` value
for the notes so that we can distinguish between "there are no notes
in the account" and "we haven yet to determine which notes are in the
account."

In comparison to #1649 we're using a tri-state value for `notes` instead
of introducing an additional flag which must be kept in sync with
`notes`.
@belcherj belcherj closed this Oct 21, 2019
@belcherj belcherj deleted the add/notes-loading-indicator branch October 21, 2019 13:08
belcherj pushed a commit that referenced this pull request Nov 4, 2019
@loremattei @belcherj
Merge release 1.10 into master (#1688)
b82a9ea 
* Update dependency concurrently to v5 (#1631)

* Update dependency electron-rebuild to v1.8.6 (#1533)

* Update dependency highlight.js to v9.15.10 (#1545)

* Update dependency autoprefixer to v9.6.4 (#1628)

* Update dependency react-overlays to v2 (#1620)

* Update dependency react-dropzone to v10.1.10 (#1630)

* Update dependency eslint-config-prettier to v6.4.0 (#1629)

* Update dependency eslint-plugin-react to v7.16.0 (#1623)

* Update react monorepo to v16.10.2 (#1621)

* Update dependency electron to v4.2.11 (#1483)

* Refactor tag operations to stop directly mutating tag objects (#1638)

See #1614

As part of a broader effort to resolve data-flow issues in the app this PR is a
first step in removing direct mutation where transactional atomic updates
should be occurring.

It's not clear if the existing code is the source of existing defects in the software
and this is part of why the code is problematic; we have created inherent
concurrency flaws that open up extremely-difficult-to-reproduce bugs.

Resolving this may or may not resolve any existing bugs but it will definitely
help guard us from introducing new ones.

---

Previously we have been directly mutating note and tag objects when
editing those tags. This mutation can lead to concurrency defects which
expose themselves as inconsistent UI state. This breaks our Redux model
which assumes that all UI updates happen atomically.

In this patch we're building new note objects and tag objects when we
make these updates in order to maintain our consistency.

---

There should be no significant visual or behavioral changes with this PR. We
are changing code related to removing tags, renaming tags, and
reordering tags.

In testing verify that with separate sessions the updates appear as expected.
Add, reorder, and remove tags to make sure the changes synchronize.

* Refactor updating note content to stop directly mutating note object (#1634)

See #1614

As part of a broader effort to resolve data-flow issues in the app this PR is a
first step in removing direct mutation where transactional atomic updates
should be occurring.

It's not clear if the existing code is the source of existing defects in the software
and this is part of why the code is problematic; we have created inherent
concurrency flaws that open up extremely-difficult-to-reproduce bugs.

Resolving this may or may not resolve any existing bugs but it will definitely
help guard us from introducing new ones.

---

Previously we have been directly mutating the note object when updating
its content. This may have been an attempt to work around confusing
data-flow issues that thankfully don't exist anymore. We have also been
performing inline checks to make sure that we update the editor's
contents if we receive these updates.

This mutation can lead to concurrency defects which expose themselves as
inconsistent UI state. This breaks our Redux model which assumes that
all UI updates happen atomically.

In this patch we're building a new note object when we update a note
in order to maintain our consistency. In light of #1598 we're also
removing some work-around code that attempted to force consistency when
it didn't exist; that consistency now exists since we're tracking the
underlying Simperium data closely now vs. storing it in separate
places.

When updating checklist items we're forcing a sync so that those changes
will propagate immediately. We don't have a need to debounce those
clicks.

* Refactor note tag operation to stop directly mutating note object (#1639)

See #1614

As part of a broader effort to resolve data-flow issues in the app this PR is a
first step in removing direct mutation where transactional atomic updates
should be occurring.

It's not clear if the existing code is the source of existing defects in the software
and this is part of why the code is problematic; we have created inherent
concurrency flaws that open up extremely-difficult-to-reproduce bugs.

Resolving this may or may not resolve any existing bugs but it will definitely
help guard us from introducing new ones.

---

Previously we have been directly mutating note objects when editing
their tags. This mutation can lead to concurrency defects which expose
themselves as inconsistent UI state. This breaks our Redux model which
assumes that all UI updates happen atomically.

In this patch we're building new note objects when we make these updates
in order to maintain our consistency.

---

There should be no significant visual or behavioral changes with this PR. We
are changing code related to removing tags, renaming tags, and
reordering tags.

In testing verify that with separate sessions the updates appear as expected.
Add, reorder, and remove tags to make sure the changes synchronize.

* Fix: Broken oAuth flow (#1627)

We have been experiencing problems when trying to login with the
WordPress.com signin. Something appears to have changed in Electron such
that the older versions of the app still work but newer versions are
failing.

In this patch we're rewriting the authentication flow to simplify it and
prepare ourselves for better handling of the failure cases.

In production we are seeing strange behaviors on failure and some on
success: unending re-requests to `simplenote://auth` which trigger full
CPU load; and no response after authentication.

After this patch we should be able to wrangle in errors and add a
timeout to better communicate when things are failing.

Additionally, the unending loop should be closed due to a replacement of
the old network intercept code with a single simplified model.

We have also been sharing sessions between the main window and the auth
window and also sharing sessions between teach time the auth window
appears.

This leads to leaked cookies and can result in confusing flows, largely
because of the shared cookies.

In this patch we're creating a new `Session` for the auth window every
time we open it. By not including `persist:` in the "partition" name
we're making sure it only exists in memory. By introducing randomness
into its name we're making sure we don't share the same session from
one auth attempt to the next. By freeing the window after close we're
making sure we don't leak memory.

Previously we were able to open the auth window after closing it and
instead of logging in again it would open to the "Accept/Deny" view.
After this change it requires logging in on every attempt. This will
likely be more frustrating but much safer than the previous behavior.

* Make system setting the default theme preference (#1581)

* Make system setting the default theme preference

* Update Release Notes

* Make system the first item in the list

* Update system theme logic

* Add option to hide menu bar (#1215)

Closes #293
Based on #1216

This adds an option to auto-hide the menu bar on Windows/Linux Electron. The option will be in Settings ▸ Display.

screen shot 2019-02-21 at 23 26 17

We have to be careful not to get the user stuck in a situation where they can't bring back the menu bar, so we'll show the Settings button and footer links in the Navigation Bar when auto-hide is enabled.

* Support the unicode bullet as a list item bullet in unordered lists (#1551)

Support the unicode bullet character • as a list item bullet because lists copied from HTML or word documents may contain this character.

* Add release note adding bullet char to lists (#1646)

* Note list: Distinguish loading from empty states (#1650)

Alternative idea to #1649

There are a few times when we boot the app and our list of notes is
empty because we haven't received udpates from the server yet. This
happens on intial app boot, immediately after authorizing, and when
we lose our local copy of the notes from `IndexedDB`.

During these times we're showing that there are no notes in the account
which is misleading. In this patch we're starting with a `null` value
for the notes so that we can distinguish between "there are no notes
in the account" and "we haven yet to determine which notes are in the
account."

In comparison to #1649 we're using a tri-state value for `notes` instead
of introducing an additional flag which must be kept in sync with
`notes`.

* Deps: Update simperium to fix infinite duplication bug

Update `simperium` library to incorporate changes that fixed a defect
where we were holding open old WebSocket connections after signing-out
and signing-in. This defect produced an infinite duplication of changes.

* Update the link to the Release Notes in the updater config (#1675)

Update the link to the Release Notes in the updater config

In (#1582) CHANGELOG.md was renamed to RELEASE-NOTES.txt. This caused the link to the release notes to break.

* In Dev mode, open CHrome Deve Tools in detached mode (#1660)

* Update version in package.json for 1.10.0 release

Added the beta version for 1.10.0-beta1 release

* Fix/only run notes loaded when notes loaded (#1680)

* Only run notesLoaded when notes are indeed loaded

After querying the noteBucket we run notes loaded with an empty notes array. This causes havoc because we rely on notes being null until notes are loaded. This commit adds a check to ensure there is at least one note before running notesLoaded

* Add release notes

* Update RELEASE-NOTES.txt

* Update signing certificate (#1682)

Props to @loremattei for creating these changes

* Bump version to v1.10.0-beta2 (#1684)

* Add version for release 1.10.0
@codebykat codebykat mentioned this pull request Nov 19, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmsnell dmsnell dmsnell left review comments

Assignees

@belcherj belcherj

Labels
None yet
Projects
None yet
Milestone
1.10
Development

Successfully merging this pull request may close these issues.
2 participants
@belcherj @dmsnell