Skip to content

Handle all m.room.aliases chunk, not only first - #270

Open
slipeer wants to merge 2 commits into
matrix-org:masterfrom
slipeer:issue106
Open

Handle all m.room.aliases chunk, not only first#270
slipeer wants to merge 2 commits into
matrix-org:masterfrom
slipeer:issue106

Conversation

@slipeer

Copy link
Copy Markdown

This is fix for #106 issue
Rooom state server reply may contain more then one m.room.aliases update_aliases() method must collect aliases from all of them.

Additionally in this code only get_room_state() can raise MatrixRequestError - there is no need to enclose to try block more strings.

Signed-off-by: Pavel Kardash slipeer@gmail.com

@slipeerslipeer changed the title Handle all m.room.aliases chunk, not only lastHandle all m.room.aliases chunk, not only firstAug 10, 2018

@non-Jedinon-Jedi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Other than comment, this LGTM. Note that this doesn't close #106 because the _process_state_event method also needs to be changed to account for multiple m.room.aliases state events.

Comment threadmatrix_client/room.py Outdated
Returns:
boolean: True if the aliases changed, False if not
"""
response = None

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm having trouble understanding why this is necessary. If try block below is successful, response will be bound to something, and if it isn't successful, the method terminates without going further.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I agree this is an excessive reinsurance

Comment threadtest/client_test.py Outdated
room.name = False
room.topic = False
room.aliases = False
room.aliases = []

@slipeerslipeerSep 3, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I found that when initializing aliases in rooms.py used empty list

, so using False here doesn't look correct.

Comment threadtest/client_test.py
ev["content"]["aliases"] = aliases
room._process_state_event(ev)
assert room.aliases is aliases
assert room.aliases == aliases

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Since now we are processing list element by element, is will no longer work here.

Comment threadtest/client_test.py
ev["type"] = "m.room.aliases"
room._process_state_event(ev)
assert room.aliases is None
assert room.aliases == []

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Since aliases is initialized as an empty list, it can no longer be None.

Comment threadtest/client_test.py

room.name = False
room.topic = False
room.aliases = False

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I found that when initializing aliases in rooms.py used empty list

, so using False here doesn't look correct.

@Zil0Zil0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks nice if not for the small bug underlined :)

Comment threadmatrix_client/room.py
return False
except MatrixRequestError:
return False
self.aliases = []

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd rather have self.aliases.clear() here.

Comment threadmatrix_client/room.py
for alias in chunk["content"]["aliases"]:
if alias not in self.aliases:
self.aliases.append(alias)
changed = True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this works as expected. Since self.aliases will always be empty because of self.aliases = [] above, changed will always be True as long as there is at least one alias, even if it's the same as before.

@Zil0

Zil0 commented Oct 4, 2018

Copy link
Copy Markdown
Contributor

Is there a reason why Room.aliases is not a set? It seems unlikely that we need ordering (as it is random anyway since it comes from a dict) or indexing, while this PR adds membership tests that could benefit a set, especially in _process_state_event. The only point I can see is to avoid confusing a user, since it is easy to assume from the look of it that an attribute called aliases is a list. Thoughts anyone?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@slipeer@Zil0@non-Jedi
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Handle all m.room.aliases chunk, not only first by slipeer · Pull Request #270 · matrix-org/matrix-python-sdk · GitHub
Skip to content

Handle all m.room.aliases chunk, not only first - #270

Open
slipeer wants to merge 2 commits into
matrix-org:masterfrom
slipeer:issue106
Open

Handle all m.room.aliases chunk, not only first#270
slipeer wants to merge 2 commits into
matrix-org:masterfrom
slipeer:issue106

Conversation

@slipeer

Copy link
Copy Markdown

This is fix for #106 issue
Rooom state server reply may contain more then one m.room.aliases update_aliases() method must collect aliases from all of them.

Additionally in this code only get_room_state() can raise MatrixRequestError - there is no need to enclose to try block more strings.

Signed-off-by: Pavel Kardash slipeer@gmail.com

@slipeerslipeer changed the title Handle all m.room.aliases chunk, not only lastHandle all m.room.aliases chunk, not only firstAug 10, 2018

@non-Jedinon-Jedi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Other than comment, this LGTM. Note that this doesn't close #106 because the _process_state_event method also needs to be changed to account for multiple m.room.aliases state events.

Comment threadmatrix_client/room.py Outdated
Returns:
boolean: True if the aliases changed, False if not
"""
response = None

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm having trouble understanding why this is necessary. If try block below is successful, response will be bound to something, and if it isn't successful, the method terminates without going further.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I agree this is an excessive reinsurance

Comment threadtest/client_test.py Outdated
room.name = False
room.topic = False
room.aliases = False
room.aliases = []

@slipeerslipeerSep 3, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I found that when initializing aliases in rooms.py used empty list

, so using False here doesn't look correct.

Comment threadtest/client_test.py
ev["content"]["aliases"] = aliases
room._process_state_event(ev)
assert room.aliases is aliases
assert room.aliases == aliases

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Since now we are processing list element by element, is will no longer work here.

Comment threadtest/client_test.py
ev["type"] = "m.room.aliases"
room._process_state_event(ev)
assert room.aliases is None
assert room.aliases == []

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Since aliases is initialized as an empty list, it can no longer be None.

Comment threadtest/client_test.py

room.name = False
room.topic = False
room.aliases = False

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I found that when initializing aliases in rooms.py used empty list

, so using False here doesn't look correct.

@Zil0Zil0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks nice if not for the small bug underlined :)

Comment threadmatrix_client/room.py
return False
except MatrixRequestError:
return False
self.aliases = []

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd rather have self.aliases.clear() here.

Comment threadmatrix_client/room.py
for alias in chunk["content"]["aliases"]:
if alias not in self.aliases:
self.aliases.append(alias)
changed = True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this works as expected. Since self.aliases will always be empty because of self.aliases = [] above, changed will always be True as long as there is at least one alias, even if it's the same as before.

@Zil0

Zil0 commented Oct 4, 2018

Copy link
Copy Markdown
Contributor

Is there a reason why Room.aliases is not a set? It seems unlikely that we need ordering (as it is random anyway since it comes from a dict) or indexing, while this PR adds membership tests that could benefit a set, especially in _process_state_event. The only point I can see is to avoid confusing a user, since it is easy to assume from the look of it that an attribute called aliases is a list. Thoughts anyone?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@slipeer@Zil0@non-Jedi
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Handle all m.room.aliases chunk, not only first by slipeer · Pull Request #270 · matrix-org/matrix-python-sdk · GitHub
Skip to content

Handle all m.room.aliases chunk, not only first - #270

Open
slipeer wants to merge 2 commits into
matrix-org:masterfrom
slipeer:issue106
Open

Handle all m.room.aliases chunk, not only first#270
slipeer wants to merge 2 commits into
matrix-org:masterfrom
slipeer:issue106

Conversation

@slipeer

Copy link
Copy Markdown

This is fix for #106 issue
Rooom state server reply may contain more then one m.room.aliases update_aliases() method must collect aliases from all of them.

Additionally in this code only get_room_state() can raise MatrixRequestError - there is no need to enclose to try block more strings.

Signed-off-by: Pavel Kardash slipeer@gmail.com

@slipeerslipeer changed the title Handle all m.room.aliases chunk, not only lastHandle all m.room.aliases chunk, not only firstAug 10, 2018

@non-Jedinon-Jedi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Other than comment, this LGTM. Note that this doesn't close #106 because the _process_state_event method also needs to be changed to account for multiple m.room.aliases state events.

Comment threadmatrix_client/room.py Outdated
Returns:
boolean: True if the aliases changed, False if not
"""
response = None

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm having trouble understanding why this is necessary. If try block below is successful, response will be bound to something, and if it isn't successful, the method terminates without going further.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I agree this is an excessive reinsurance

Comment threadtest/client_test.py Outdated
room.name = False
room.topic = False
room.aliases = False
room.aliases = []

@slipeerslipeerSep 3, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I found that when initializing aliases in rooms.py used empty list

, so using False here doesn't look correct.

Comment threadtest/client_test.py
ev["content"]["aliases"] = aliases
room._process_state_event(ev)
assert room.aliases is aliases
assert room.aliases == aliases

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Since now we are processing list element by element, is will no longer work here.

Comment threadtest/client_test.py
ev["type"] = "m.room.aliases"
room._process_state_event(ev)
assert room.aliases is None
assert room.aliases == []

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Since aliases is initialized as an empty list, it can no longer be None.

Comment threadtest/client_test.py

room.name = False
room.topic = False
room.aliases = False

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I found that when initializing aliases in rooms.py used empty list

, so using False here doesn't look correct.

@Zil0Zil0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks nice if not for the small bug underlined :)

Comment threadmatrix_client/room.py
return False
except MatrixRequestError:
return False
self.aliases = []

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd rather have self.aliases.clear() here.

Comment threadmatrix_client/room.py
for alias in chunk["content"]["aliases"]:
if alias not in self.aliases:
self.aliases.append(alias)
changed = True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this works as expected. Since self.aliases will always be empty because of self.aliases = [] above, changed will always be True as long as there is at least one alias, even if it's the same as before.

@Zil0

Zil0 commented Oct 4, 2018

Copy link
Copy Markdown
Contributor

Is there a reason why Room.aliases is not a set? It seems unlikely that we need ordering (as it is random anyway since it comes from a dict) or indexing, while this PR adds membership tests that could benefit a set, especially in _process_state_event. The only point I can see is to avoid confusing a user, since it is easy to assume from the look of it that an attribute called aliases is a list. Thoughts anyone?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@slipeer@Zil0@non-Jedi
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Handle all m.room.aliases chunk, not only first by slipeer · Pull Request #270 · matrix-org/matrix-python-sdk · GitHub
Skip to content

Handle all m.room.aliases chunk, not only first - #270

Open
slipeer wants to merge 2 commits into
matrix-org:masterfrom
slipeer:issue106
Open

Handle all m.room.aliases chunk, not only first#270
slipeer wants to merge 2 commits into
matrix-org:masterfrom
slipeer:issue106

Conversation

@slipeer

Copy link
Copy Markdown

This is fix for #106 issue
Rooom state server reply may contain more then one m.room.aliases update_aliases() method must collect aliases from all of them.

Additionally in this code only get_room_state() can raise MatrixRequestError - there is no need to enclose to try block more strings.

Signed-off-by: Pavel Kardash slipeer@gmail.com

@slipeerslipeer changed the title Handle all m.room.aliases chunk, not only lastHandle all m.room.aliases chunk, not only firstAug 10, 2018

@non-Jedinon-Jedi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Other than comment, this LGTM. Note that this doesn't close #106 because the _process_state_event method also needs to be changed to account for multiple m.room.aliases state events.

Comment threadmatrix_client/room.py Outdated
Returns:
boolean: True if the aliases changed, False if not
"""
response = None

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm having trouble understanding why this is necessary. If try block below is successful, response will be bound to something, and if it isn't successful, the method terminates without going further.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I agree this is an excessive reinsurance

Comment threadtest/client_test.py Outdated
room.name = False
room.topic = False
room.aliases = False
room.aliases = []

@slipeerslipeerSep 3, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I found that when initializing aliases in rooms.py used empty list

, so using False here doesn't look correct.

Comment threadtest/client_test.py
ev["content"]["aliases"] = aliases
room._process_state_event(ev)
assert room.aliases is aliases
assert room.aliases == aliases

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Since now we are processing list element by element, is will no longer work here.

Comment threadtest/client_test.py
ev["type"] = "m.room.aliases"
room._process_state_event(ev)
assert room.aliases is None
assert room.aliases == []

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Since aliases is initialized as an empty list, it can no longer be None.

Comment threadtest/client_test.py

room.name = False
room.topic = False
room.aliases = False

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I found that when initializing aliases in rooms.py used empty list

, so using False here doesn't look correct.

@Zil0Zil0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks nice if not for the small bug underlined :)

Comment threadmatrix_client/room.py
return False
except MatrixRequestError:
return False
self.aliases = []

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd rather have self.aliases.clear() here.

Comment threadmatrix_client/room.py
for alias in chunk["content"]["aliases"]:
if alias not in self.aliases:
self.aliases.append(alias)
changed = True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this works as expected. Since self.aliases will always be empty because of self.aliases = [] above, changed will always be True as long as there is at least one alias, even if it's the same as before.

@Zil0

Zil0 commented Oct 4, 2018

Copy link
Copy Markdown
Contributor

Is there a reason why Room.aliases is not a set? It seems unlikely that we need ordering (as it is random anyway since it comes from a dict) or indexing, while this PR adds membership tests that could benefit a set, especially in _process_state_event. The only point I can see is to avoid confusing a user, since it is easy to assume from the look of it that an attribute called aliases is a list. Thoughts anyone?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@slipeer@Zil0@non-Jedi
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' Handle all m.room.aliases chunk, not only first by slipeer · Pull Request #270 · matrix-org/matrix-python-sdk · GitHub
Skip to content

Handle all m.room.aliases chunk, not only first - #270

Open
slipeer wants to merge 2 commits into
matrix-org:masterfrom
slipeer:issue106
Open

Handle all m.room.aliases chunk, not only first#270
slipeer wants to merge 2 commits into
matrix-org:masterfrom
slipeer:issue106

Conversation

@slipeer

Copy link
Copy Markdown

This is fix for #106 issue
Rooom state server reply may contain more then one m.room.aliases update_aliases() method must collect aliases from all of them.

Additionally in this code only get_room_state() can raise MatrixRequestError - there is no need to enclose to try block more strings.

Signed-off-by: Pavel Kardash slipeer@gmail.com

@slipeerslipeer changed the title Handle all m.room.aliases chunk, not only lastHandle all m.room.aliases chunk, not only firstAug 10, 2018

@non-Jedinon-Jedi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Other than comment, this LGTM. Note that this doesn't close #106 because the _process_state_event method also needs to be changed to account for multiple m.room.aliases state events.

Comment threadmatrix_client/room.py Outdated
Returns:
boolean: True if the aliases changed, False if not
"""
response = None

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm having trouble understanding why this is necessary. If try block below is successful, response will be bound to something, and if it isn't successful, the method terminates without going further.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I agree this is an excessive reinsurance

Comment threadtest/client_test.py Outdated
room.name = False
room.topic = False
room.aliases = False
room.aliases = []

@slipeerslipeerSep 3, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I found that when initializing aliases in rooms.py used empty list

, so using False here doesn't look correct.

Comment threadtest/client_test.py
ev["content"]["aliases"] = aliases
room._process_state_event(ev)
assert room.aliases is aliases
assert room.aliases == aliases

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Since now we are processing list element by element, is will no longer work here.

Comment threadtest/client_test.py
ev["type"] = "m.room.aliases"
room._process_state_event(ev)
assert room.aliases is None
assert room.aliases == []

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Since aliases is initialized as an empty list, it can no longer be None.

Comment threadtest/client_test.py

room.name = False
room.topic = False
room.aliases = False

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I found that when initializing aliases in rooms.py used empty list

, so using False here doesn't look correct.

@Zil0Zil0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks nice if not for the small bug underlined :)

Comment threadmatrix_client/room.py
return False
except MatrixRequestError:
return False
self.aliases = []

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd rather have self.aliases.clear() here.

Comment threadmatrix_client/room.py
for alias in chunk["content"]["aliases"]:
if alias not in self.aliases:
self.aliases.append(alias)
changed = True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this works as expected. Since self.aliases will always be empty because of self.aliases = [] above, changed will always be True as long as there is at least one alias, even if it's the same as before.

@Zil0

Zil0 commented Oct 4, 2018

Copy link
Copy Markdown
Contributor

Is there a reason why Room.aliases is not a set? It seems unlikely that we need ordering (as it is random anyway since it comes from a dict) or indexing, while this PR adds membership tests that could benefit a set, especially in _process_state_event. The only point I can see is to avoid confusing a user, since it is easy to assume from the look of it that an attribute called aliases is a list. Thoughts anyone?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@slipeer@Zil0@non-Jedi
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Handle all m.room.aliases chunk, not only first by slipeer · Pull Request #270 · matrix-org/matrix-python-sdk · GitHub
Skip to content

Handle all m.room.aliases chunk, not only first - #270

Open
slipeer wants to merge 2 commits into
matrix-org:masterfrom
slipeer:issue106
Open

Handle all m.room.aliases chunk, not only first#270
slipeer wants to merge 2 commits into
matrix-org:masterfrom
slipeer:issue106

Conversation

@slipeer

Copy link
Copy Markdown

This is fix for #106 issue
Rooom state server reply may contain more then one m.room.aliases update_aliases() method must collect aliases from all of them.

Additionally in this code only get_room_state() can raise MatrixRequestError - there is no need to enclose to try block more strings.

Signed-off-by: Pavel Kardash slipeer@gmail.com

@slipeerslipeer changed the title Handle all m.room.aliases chunk, not only lastHandle all m.room.aliases chunk, not only firstAug 10, 2018

@non-Jedinon-Jedi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Other than comment, this LGTM. Note that this doesn't close #106 because the _process_state_event method also needs to be changed to account for multiple m.room.aliases state events.

Comment threadmatrix_client/room.py Outdated
Returns:
boolean: True if the aliases changed, False if not
"""
response = None

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm having trouble understanding why this is necessary. If try block below is successful, response will be bound to something, and if it isn't successful, the method terminates without going further.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I agree this is an excessive reinsurance

Comment threadtest/client_test.py Outdated
room.name = False
room.topic = False
room.aliases = False
room.aliases = []

@slipeerslipeerSep 3, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I found that when initializing aliases in rooms.py used empty list

, so using False here doesn't look correct.

Comment threadtest/client_test.py
ev["content"]["aliases"] = aliases
room._process_state_event(ev)
assert room.aliases is aliases
assert room.aliases == aliases

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Since now we are processing list element by element, is will no longer work here.

Comment threadtest/client_test.py
ev["type"] = "m.room.aliases"
room._process_state_event(ev)
assert room.aliases is None
assert room.aliases == []

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Since aliases is initialized as an empty list, it can no longer be None.

Comment threadtest/client_test.py

room.name = False
room.topic = False
room.aliases = False

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I found that when initializing aliases in rooms.py used empty list

, so using False here doesn't look correct.

@Zil0Zil0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks nice if not for the small bug underlined :)

Comment threadmatrix_client/room.py
return False
except MatrixRequestError:
return False
self.aliases = []

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd rather have self.aliases.clear() here.

Comment threadmatrix_client/room.py
for alias in chunk["content"]["aliases"]:
if alias not in self.aliases:
self.aliases.append(alias)
changed = True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this works as expected. Since self.aliases will always be empty because of self.aliases = [] above, changed will always be True as long as there is at least one alias, even if it's the same as before.

@Zil0

Zil0 commented Oct 4, 2018

Copy link
Copy Markdown
Contributor

Is there a reason why Room.aliases is not a set? It seems unlikely that we need ordering (as it is random anyway since it comes from a dict) or indexing, while this PR adds membership tests that could benefit a set, especially in _process_state_event. The only point I can see is to avoid confusing a user, since it is easy to assume from the look of it that an attribute called aliases is a list. Thoughts anyone?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@slipeer@Zil0@non-Jedi
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); })(); Handle all m.room.aliases chunk, not only first by slipeer · Pull Request #270 · matrix-org/matrix-python-sdk · GitHub
Skip to content

Handle all m.room.aliases chunk, not only first - #270

Open
slipeer wants to merge 2 commits into
matrix-org:masterfrom
slipeer:issue106
Open

Handle all m.room.aliases chunk, not only first#270
slipeer wants to merge 2 commits into
matrix-org:masterfrom
slipeer:issue106

Conversation

@slipeer

Copy link
Copy Markdown

This is fix for #106 issue
Rooom state server reply may contain more then one m.room.aliases update_aliases() method must collect aliases from all of them.

Additionally in this code only get_room_state() can raise MatrixRequestError - there is no need to enclose to try block more strings.

Signed-off-by: Pavel Kardash slipeer@gmail.com

@slipeerslipeer changed the title Handle all m.room.aliases chunk, not only lastHandle all m.room.aliases chunk, not only firstAug 10, 2018

@non-Jedinon-Jedi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Other than comment, this LGTM. Note that this doesn't close #106 because the _process_state_event method also needs to be changed to account for multiple m.room.aliases state events.

Comment threadmatrix_client/room.py Outdated
Returns:
boolean: True if the aliases changed, False if not
"""
response = None

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm having trouble understanding why this is necessary. If try block below is successful, response will be bound to something, and if it isn't successful, the method terminates without going further.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I agree this is an excessive reinsurance

Comment threadtest/client_test.py Outdated
room.name = False
room.topic = False
room.aliases = False
room.aliases = []

@slipeerslipeerSep 3, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I found that when initializing aliases in rooms.py used empty list

, so using False here doesn't look correct.

Comment threadtest/client_test.py
ev["content"]["aliases"] = aliases
room._process_state_event(ev)
assert room.aliases is aliases
assert room.aliases == aliases

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Since now we are processing list element by element, is will no longer work here.

Comment threadtest/client_test.py
ev["type"] = "m.room.aliases"
room._process_state_event(ev)
assert room.aliases is None
assert room.aliases == []

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Since aliases is initialized as an empty list, it can no longer be None.

Comment threadtest/client_test.py

room.name = False
room.topic = False
room.aliases = False

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I found that when initializing aliases in rooms.py used empty list

, so using False here doesn't look correct.

@Zil0Zil0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks nice if not for the small bug underlined :)

Comment threadmatrix_client/room.py
return False
except MatrixRequestError:
return False
self.aliases = []

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd rather have self.aliases.clear() here.

Comment threadmatrix_client/room.py
for alias in chunk["content"]["aliases"]:
if alias not in self.aliases:
self.aliases.append(alias)
changed = True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this works as expected. Since self.aliases will always be empty because of self.aliases = [] above, changed will always be True as long as there is at least one alias, even if it's the same as before.

@Zil0

Zil0 commented Oct 4, 2018

Copy link
Copy Markdown
Contributor

Is there a reason why Room.aliases is not a set? It seems unlikely that we need ordering (as it is random anyway since it comes from a dict) or indexing, while this PR adds membership tests that could benefit a set, especially in _process_state_event. The only point I can see is to avoid confusing a user, since it is easy to assume from the look of it that an attribute called aliases is a list. Thoughts anyone?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@slipeer@Zil0@non-Jedi