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

When the org-web tab becomes visible, pull the latest file #65

Merged
merged 10 commits into from Aug 10, 2019

Conversation

@munen
Copy link
Contributor

commented Aug 5, 2019

This is rather helpful when the app is used in production. Since the production build supports loading the complete application and org-file from cache, it can be open for a very long time. When the org-file hasn't been pulled in a "very long time"™, then chances are non-nil that the file has been changed by another client in the meantime.

  Without this change, when the user opens the app after a while, makes changes to the file and wants to sync to the back-end, there might be the message "Since you last pulled, a newer version of the file has been pushed to the server.". Now the user has two conflicting versions of the same file and can only chose to keep one (Which in itself is great UX and great error handling for cases in which we do encounter a merge conflict, of course!).

  This situation is mitigated with this change. Now the user has the option to enable "Sync on application becoming visible" which acts similarly to "Live Sync". When the user opts to use this feature, whenever the application get's pulled from the background or started through the service worker, the first thing that happens it that a new version of the org-file is pulled from the server. It's therefore much harder for the user to create conflicts.
  
  Note: This works for me on the desktop. I tested on Chromium and Firefox.

munen added some commits Aug 4, 2019

test: Write test showing that parser breaks indentation
      This is a first step to resolve #56
fix: parser doesn't change indentation for basic lists
     However, if there's also a planning item in the description, the
     bug still persists.
feat: When the org-web tab becomes visible, pull the latest file
      This is rather helpful when the app is used in production. Since
      the production build supports loading the complete application
      and org-file from cache, it can be open for a very long time.
      When the org-file hasn't been pulled in a "very long time"™,
      then chances are non-nil that the file has been changed by
      another client in the meantime.

      Without this change, when the user opens the app after a while,
      makes changes to the file and wants to sync to the back-end,
      there might be the message "Since you last pulled, a newer
      version of the file has been pushed to the server.". Now the
      user has two conflicting versions of the same file and can only
      chose to keep one (Which in itself is great UX and great error
      handling for cases in which we do encounter a merge conflict, of
      course!).

      This situation is mitigated with this change. Now the user has
      the option to enable "Sync on application becoming visible"
      which acts similarly to "Live Sync". When the user opts to use
      this feature, whenever the application get's pulled from the
      background or started through the service worker, the first
      thing that happens it that a new version of the org-file is
      pulled from the server. It's therefore much harder for the user
      to create conflicts.
@DanielDe
Copy link
Owner

left a comment

Just a few minor comments here!

import { sync } from '../actions/org';
import { debounce } from 'lodash';

function dispatchSync(store) {

This comment has been minimized.

Copy link
@DanielDe

DanielDe Aug 7, 2019

Owner

Hey @munen, sorry to nit, but I've had a pretty strong preference for arrow functions in org-web instead of old-style functions. So something like this:

const dispatchSync = store => store.dispatch(sync({ shouldSuppressMessages: true }));

Also, have you installed Prettier? I noticed an indentation issue above that I think Prettier would've picked up on

This comment has been minimized.

Copy link
@munen

munen Aug 7, 2019

Author Contributor

@DanielDe Nitting is good. Those are my first of hopefully many contributions. It's good if we get a consistent feel on how the codebase should evolve. Just keep commenting^^

I'll gladly change it to a arrow function - and have a suggestion: What do you think of having an extensive eslintrc file with suggestions like these? This particular one isn't in the standard eslint functionality, though (only for callbacks).

Regarding prettier, I use it myself (see my autoformat Emacs integration):

Also, I read your contribution guidelines^^. Running it on the file yields no changes to me.

What’s the indentation issue you were referring to?

This comment has been minimized.

Copy link
@munen

munen Aug 7, 2019

Author Contributor

Done.

This comment has been minimized.

Copy link
@DanielDe

DanielDe Aug 10, 2019

Owner

The indentation issue was in a different file, but I'll look into it. Maybe I was just wrong about how Prettier would format that!

I'm not a huge fan of linters myself, though I'm open to it if it seems necessary in the future.

// the event listener is registered multiple times. However, this
// is not the case:
// https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#Multiple_identical_event_listeners
window.addEventListener('visibilitychange', function() {

This comment has been minimized.

Copy link
@DanielDe

DanielDe Aug 7, 2019

Owner

Likewise here, I'd love to change this to an arrow function!

This comment has been minimized.

Copy link
@munen

munen Aug 7, 2019

Author Contributor

Done.

<div className="setting-label">
Sync on application becoming visible
<div className="setting-label__description">
If enabled, the current org file is pulled from to the sync backend when the browser tab becomes visible. This prevents you from having a stale file before starting to make changes to it.

This comment has been minimized.

Copy link
@DanielDe

DanielDe Aug 7, 2019

Owner

Minor typo here: ...from to the sync... should be ...from the sync...

This comment has been minimized.

Copy link
@munen

munen Aug 7, 2019

Author Contributor

Good catch. Thanks!

Done.

munen added some commits Aug 7, 2019

chore: Code review
       - Arrow functions instead of old school `function`s
       - Typo

@DanielDe DanielDe merged commit a8a3bab into DanielDe:master Aug 10, 2019

@DanielDe

This comment has been minimized.

Copy link
Owner

commented Aug 10, 2019

Awesome stuff, thanks again @munen. I'm going to review and merge your others before deploying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.