Follow narrows from old->new stream names #4722
Conversation
|
Automated message from Dropbox CLA bot @jmandel, it looks like you've already signed the Dropbox CLA. Thanks! |
| @@ -2331,7 +2331,7 @@ def test_subscriber(self): | |||
| def test_gather_subscriptions(self): | |||
| # type: () -> None | |||
| """ | |||
| gather_subscriptions returns correct results with only 3 queries | |||
There was a problem hiding this comment.
Note that the 3 in this comment was already out of sync (was really 4 already).
jmandel
force-pushed
the
forward-narrows
branch
2 times, most recently
from
40fd583 to
541d8ad
Compare
| // (it's already tracking a page load), we process on a future tick. | ||
| // We set the "replace_state" parameter true to avoid polluting history. | ||
| setTimeout(function () { | ||
| hashchange.save_narrow(operators, true); |
There was a problem hiding this comment.
hmm, ideally, we'd be able to do this by rewriting the narrow in the hashchange.js on-page-load code path, rather than loading the old narrow and then flickering it to the new state.
There was a problem hiding this comment.
This loads the new narrow directly (the Filter has been intercepted and updated by the time we reach this line, in the first call to narrow.activate). It's just the hash that updates in the next tick; the data are already loaded. In testing this, I don't see any flicker.
I've looked at moving this logic to onload (have this implemented locally; can share), but some concerns:
- hashchange.js is indeed thorny. I don't love adding more logic to the
changing_hashdetection - moving logic to onload means that typing an old stream name into the search box won't forward it; this leads to an inconsistency where Zulip displays an empty result set when a user types an old stream name into the search box (e.g. "stream:OldName"), but the upon hitting Ctrl-R the forward happens, the URL updates, and content appears.
Thoughts @timabbott ?
There was a problem hiding this comment.
I see, if it's just the hash that's updating in the next tick, that's pretty invisible and fine. I just don't want a flicker from the "stream doesn't exist" notice and back.
BTW http://zulip.readthedocs.io/en/latest/hashchange-system.html may be useful reading if you haven't found it, but I 100% agree that the system is thorny and could benefit from a restructuring.
@showell what are your thoughts? If you have time to take over this review in general, that'd be great.
There was a problem hiding this comment.
Yeah, I borrowed the word "thorny" from that doc (very helpful orientatation, by the way) :)
In any case, to confirm: the "stream doesn't exist" message never appears in this implementation, when following an old alias to an new stram name. @showell, would love your review!
zulipbot
added
needs review
needs more work
and removed
needs more work
needs review
labels
Remove aliases for re-created streams Add missing migration Simplify logic now that old names can only appear once Bump query count now that we .delete() node tests Pass tests Smarter alias detection Clean up js Use underscore for map/filter Update stream_data.js Remove outdated aliases to new steram name
|
I added some comments to the issue here. Let's discuss this on chat before getting too deep into the details of the PR. |
|
Heads up @jmandel, we just merged some commits (latest: e6cc0ff) that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the |
|
There are three components to this fix:
The 2nd/3rd items here are yet to be done, and they're somewhat covered in this PR. |
|
Unless @jmandel objects, I think we should close this for now, since it will still be linked back from the original issue. The PR has useful bits, but my work right before PyCon makes some of the JS-side stuff no longer needed. |
|
Yeah, no problem! I wish I had more time to see this through :/ |
|
Oops, I didn't mean to close this earlier, but I'll leave it closed based on Josh's comments. We'll probably get to this some time this summer, especially as some of our summer folks get up to speed on live-update issues. |
This is to address #4713. As I mentioned there, there may not be much worth keeping in this approach, but feedback would be welcome!