-
-
Notifications
You must be signed in to change notification settings - Fork 643
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
Fix do not mark as read, and make it more visible #4880
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am only partially removing things related to debug settings (like DebugScreen
, and debug
property inside BackgroundData
) since I feel like the code may be reusable when we add another debug setting in the future.
We can squash the last 2 commits, I have made them separate so that It will be easy to review. Let me know if this is required or not.
It would be great if someone reviewing this can take a look at a few doubts I had (see below)
src/boot/store.js
Outdated
@@ -99,6 +99,7 @@ function dropCache(state: GlobalState): $Shape<GlobalState> { | |||
// $FlowFixMe[prop-missing] | |||
// $FlowFixMe[incompatible-variance] | |||
// $FlowFixMe[incompatible-type-arg] | |||
// $FlowFixMe[incompatible-shape] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an appropriate thing to do? I have made the type Debug = $Shape<{||}>
and flow was giving incompatible shape error on that, I am assuming here that we won't pass through SessionState
(which contains debug: Debug
) here because storeKeys
doesn't contain that value and hence we can ignore the error related to that. Is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I think the answer here is to not use $Shape
:
- In general,
$Shape
has some funny unsound behavior (https://flow.org/en/docs/types/utilities/#toc-shape ) and so we usually avoid it. - Here,
{||}
already means exactly the type of objects with no properties. So there isn't anything for$Shape
to do, other than its funny unsoundness.
It looks like if I delete the $Shape
I get an error at the point where it's initialized to {}
. That is a known Flow quirk, where the empty object literal {}
can't be used as the empty-object type {||}
. Fortunately there is a clean workaround, which is to say Object.freeze({})
instead: facebook/flow#2386 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know! thanks.
src/webview/js/js.js
Outdated
@@ -49,6 +49,8 @@ import { toggleSpoiler } from './spoilers'; | |||
*/ | |||
declare var platformOS: string; | |||
|
|||
let doNotMarkMessagesAsReadFlag = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am using a global variable here since we need to use this in multiple functions inside js.js
, it is defined within handleInitialLoad
and used within sendScrollMessage
, Is that appropriate? does there exists some better design pattern that I can follow here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good question.
I think a somewhat better pattern would be to follow what we do for platformOS
:
- It's defined directly in
script.js
, before the code incompiledWebviewJs
(which has all the code injs.js
and its friends) runs. This means it's initialized with the correct value from the beginning, and there's no need to worry about initialization order. - Then there's a
declare var
line here at the top ofjs.js
, so that Flow knows about it.
That's still kind of awkward, because there's no type-checking that what we do in script.js
matches what js.js
expects. But that's equally true with the parameters we pass to handleInitialLoad
.
I think having it in a global is fine, so long as it has a nice explicit name like this. There's kind of an underlying messiness in the fact that this file is pretty long and does quite a lot of different things. So that makes it kind of uncomfortable to add more things to its top-level namespace. But I think the main thing for us to do there is to take some of the larger clusters of related code in this file and move them to separate modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
passing it like platformOS
in new revision.
Thanks @AkashDhiman ! I replied to your inline questions above.
Yeah, I think those two changes will be best squashed into one commit. That's because they have the nature of removing a thing, and then adding back a revised version of the same thing. When squashed together, both halves of that change become easier to understand and review, because the whole thing can be seen as some moving and editing. The other branch-level thing I see is that the first commit adds some plumbing to bring the whole I have some small code-level comments too, which I'll make inline. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done reading through this; inline comments below. Thanks again!
test('DEBUG_FLAG_TOGGLE', () => { | ||
const action = deepFreeze({ type: DEBUG_FLAG_TOGGLE, key: 'someKey', value: true }); | ||
expect(sessionReducer(baseState, action)).toEqual({ | ||
...baseState, | ||
debug: { doNotMarkMessagesAsRead: false, someKey: true }, | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a test for some logic we still have, in sessionReducer.js
. So we should delete it only when we're actually deleting that logic.
That'll be a separate commit from any of the changes in this branch -- basically we'd have a commit ripping out the Debug
type and DebugScreen
, and all the remaining references to those. I think it'd be pretty reasonable to include that at the end of this PR, but I'm also happy to let it sit around for a bit and see if we find another use for it soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added back, I am keeping debug
related code as it is, for potential future use.
@@ -295,6 +295,7 @@ export type SettingsState = {| | |||
experimentalFeaturesEnabled: boolean, | |||
streamNotification: boolean, | |||
browser: BrowserPreference, | |||
doNotMarkMessagesAsRead: boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're making a change to the Redux state type, so we should have a migration for it.
(More specifically, it's in a subtree that we actually store between runs -- see storeKeys
and cacheKeys
in src/boot/store.js
. This didn't apply to the change to SessionState
via changing Debug
, because state.session
isn't stored; it's in discardKeys
.)
Actually, we had a recent discussion where the conclusion was that maybe we shouldn't quite have a migration in this case, because the way the persisted state gets merged with the reducer's initial state will have the right effect already. But still we should have a comment in the list of migrations, saying what changed and that we explicitly decided no migration was needed.
(I'm regretting now that that discussion was on GitHub instead of Zulip, though -- it may not be easy to find.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a difference when using dropCache
vs returning a modified state like some migrations are doing? example - '28'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/data.20migrations/near/1230029 (same link as below, but here too for cross-reference)
src/webview/js/js.js
Outdated
|
||
doNotMarkMessagesAsReadFlag = doNotMarkMessagesAsRead; | ||
// Since its version 5.x, the `react-native-webview` library dispatches our |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: keep the blank line -- it's serving a purpose by separating this comment from what's above, so that it's visually attached to the whole stanza below
37232a3
to
f01d45e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the revision!
All looks good, except the migration (which you asked about above.) I'll fix that up and merge.
src/boot/store.js
Outdated
@@ -302,6 +302,9 @@ const migrations: {| [string]: (GlobalState) => GlobalState |} = { | |||
outbox: state.outbox.filter(o => o.sender_id !== undefined), | |||
}), | |||
|
|||
// Add `doNotMarkMessagesAsRead` in `SettingsState`. | |||
'30': dropCache, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't the right thing here; it actually has no effect on the settings
state. (You asked about this line in a subthread above, too.)
I guess I just said above that I regretted having a previous discussion on a GitHub PR thread instead of in Zulip because it's harder to find here, so I'll make a chat thread to go into more detail 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done by: - passing `doNotMarkMessagesAsRead` (a debug flag) from: `MessageList` -> `html` -> `script` -> `js` - Injecting it predefined in `script.js` - Using it to conditionally call `setMessagesReadAttribute`, which is responsible for erasing the left unread indicator. Fixes: zulip#4849
Previously it was present at `debug settings`, moving this to general settings makes it more discoverable. Note that this commit makes debug settings useless, since it contained only this one setting, code related to it is not removed however for potential future use. Fixes: zulip#4850
f01d45e
to
6359d1a
Compare
Done by first passing
doNotMarkAsRead
information to webview thenmigrating
doNotMarkAsRead
from debug setting to general setting.Fixes: #4849
Fixes: #4850