ref: Extract general chunk uploading logic from upload_difs_chunked - #2303

Closed
szokeasaurusrex wants to merge 2 commits into
masterfrom
szokeasaurusrex/proguard-upload-10
Closed

ref: Extract general chunk uploading logic from upload_difs_chunked#2303
szokeasaurusrex wants to merge 2 commits into
masterfrom
szokeasaurusrex/proguard-upload-10

Conversation

@szokeasaurusrex

Copy link
Copy Markdown
Member

The upload_difs_chunked function contains some code which is specific to uploading difs, and some code which is general to uploading chunked objects. This change extracts the code that is generic to chunked objects

@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from a661c6c to 6203e7aCompareDecember 5, 2024 15:34
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-10 branch from d4736a9 to e4cd295CompareDecember 5, 2024 15:34
@szokeasaurusrexszokeasaurusrex changed the title ref: Extract general chunk uploadin logic from upload_difs_chunkedref: Extract general chunk uploading logic from upload_difs_chunkedDec 5, 2024
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-10 branch from e4cd295 to 8caf2b4CompareDecember 5, 2024 15:36
Comment on lines +336 to +346
fn render_detail(detail: &Option<String>, fallback: Option<&str>) {
let mut string = match *detail {
Some(ref string) => string.as_str(),
None => "",
};

if string.is_empty() {
if let Some(fallback) = fallback {
string = fallback;
}
}

@loewenheimloewenheimDec 5, 2024

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.

  1. &Option<…> in a function signature is kind of a code smell; you can always do better by taking Option<&…> instead. You just have to call it with .as_ref(), or in this case .as_deref().
  2. The logic of picking the detail or the fallback can be expressed more clearly.
Suggested change
fn render_detail(detail:&Option<String>,fallback:Option<&str>){
letmut string = match*detail {
Some(ref string) => string.as_str(),
None => "",
};
if string.is_empty(){
ifletSome(fallback) = fallback {
string = fallback;
}
}
fn render_detail(detail:Option<&str>,fallback:Option<&str>){
let detail = detail.unwrap_or_default();
let fallback = fallback.unwrap_or_default();
let string = if detail.is_empty(){ fallback }else{ detail };

To expand on (1) a bit: Option<&T> is strictly more general than &Option<T>; there is nothing you can do with the latter that you couldn't do with the former, but &Option<T> requires you to actually have an Option<T> somewhere that you can take a reference of.

Similarly, there's no reason to use &String over &str. &str will let you do anything that &String would, but you don't need an owned String to use it.

Combining the two, you should use Option<&str> over &Option<String>. At the call site you can use .as_deref() to get exactly the type you need.

@szokeasaurusrexszokeasaurusrexDec 6, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Fair point, but I will do this change in a separate PR. I am simply copying the existing function from dif_upload.rs in this PR

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.

Oh, I didn't realize this was existing code.

fn server_options(&self) -> &ChunkServerOptions;

/// Uploads the given chunked objects to the server.
fn upload_chunked_objects<T>(self, chunked: &[Chunked<T>]) -> Result<(Vec<DebugInfoFile>, bool)>

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 wouldn't do this as a method on this trait; it seems to me that this should be a free function, although this is admittedly based on an aesthetic feeling.

Add functionality to the `ChunkOptions` trait, allowing the `ChunkServerOptions` to be obtained. With this change, `DifUpload` can no longer implement `ChunkOptions` directly, since it does not contain the server options, so we create a new `ChunkedDifUpload` struct to implement `ChunkOptions` by acting as a container for the `DifUpload` and `ChunkServerOptions`.
Also, we take advantage of the new functionality by refactoring `upload_difs_chunked` to take only a single argument, the `ChunkedDifUpload`. The upload methods now take owned options structs, since an upload should only be performed once.
The `upload_difs_chunked` function contains some code which is specific to uploading difs, and some code which is general to uploading chunked objects. This change extracts the code that is generic to chunked objects
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from 6203e7a to d923964CompareDecember 6, 2024 13:39
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-10 branch from 8caf2b4 to 73ec38eCompareDecember 6, 2024 13:40
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from d923964 to cf02c17CompareDecember 16, 2024 15:28
@szokeasaurusrex
szokeasaurusrex marked this pull request as draft December 16, 2024 15:31
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from cf02c17 to 5a78717CompareDecember 16, 2024 15:38
Base automatically changed from szokeasaurusrex/proguard-upload-9 to masterDecember 16, 2024 15:49
@szokeasaurusrex
szokeasaurusrex deleted the szokeasaurusrex/proguard-upload-10 branch December 16, 2024 15:55
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@szokeasaurusrex@loewenheim
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all \u003cpre\u003e\u003ccode\u003e blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n 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;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks"); } } catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); } })(); (function(){ try { var __m = "github.com"; var __re = new RegExp('^' + "github\\.com" + '
Skip to content

ref: Extract general chunk uploading logic from upload_difs_chunked - #2303

Closed
szokeasaurusrex wants to merge 2 commits into
masterfrom
szokeasaurusrex/proguard-upload-10
Closed

ref: Extract general chunk uploading logic from upload_difs_chunked#2303
szokeasaurusrex wants to merge 2 commits into
masterfrom
szokeasaurusrex/proguard-upload-10

Conversation

@szokeasaurusrex

Copy link
Copy Markdown
Member

The upload_difs_chunked function contains some code which is specific to uploading difs, and some code which is general to uploading chunked objects. This change extracts the code that is generic to chunked objects

@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from a661c6c to 6203e7aCompareDecember 5, 2024 15:34
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-10 branch from d4736a9 to e4cd295CompareDecember 5, 2024 15:34
@szokeasaurusrexszokeasaurusrex changed the title ref: Extract general chunk uploadin logic from upload_difs_chunkedref: Extract general chunk uploading logic from upload_difs_chunkedDec 5, 2024
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-10 branch from e4cd295 to 8caf2b4CompareDecember 5, 2024 15:36
Comment on lines +336 to +346
fn render_detail(detail: &Option<String>, fallback: Option<&str>) {
let mut string = match *detail {
Some(ref string) => string.as_str(),
None => "",
};

if string.is_empty() {
if let Some(fallback) = fallback {
string = fallback;
}
}

@loewenheimloewenheimDec 5, 2024

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.

  1. &Option<…> in a function signature is kind of a code smell; you can always do better by taking Option<&…> instead. You just have to call it with .as_ref(), or in this case .as_deref().
  2. The logic of picking the detail or the fallback can be expressed more clearly.
Suggested change
fn render_detail(detail:&Option<String>,fallback:Option<&str>){
letmut string = match*detail {
Some(ref string) => string.as_str(),
None => "",
};
if string.is_empty(){
ifletSome(fallback) = fallback {
string = fallback;
}
}
fn render_detail(detail:Option<&str>,fallback:Option<&str>){
let detail = detail.unwrap_or_default();
let fallback = fallback.unwrap_or_default();
let string = if detail.is_empty(){ fallback }else{ detail };

To expand on (1) a bit: Option<&T> is strictly more general than &Option<T>; there is nothing you can do with the latter that you couldn't do with the former, but &Option<T> requires you to actually have an Option<T> somewhere that you can take a reference of.

Similarly, there's no reason to use &String over &str. &str will let you do anything that &String would, but you don't need an owned String to use it.

Combining the two, you should use Option<&str> over &Option<String>. At the call site you can use .as_deref() to get exactly the type you need.

@szokeasaurusrexszokeasaurusrexDec 6, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Fair point, but I will do this change in a separate PR. I am simply copying the existing function from dif_upload.rs in this PR

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.

Oh, I didn't realize this was existing code.

fn server_options(&self) -> &ChunkServerOptions;

/// Uploads the given chunked objects to the server.
fn upload_chunked_objects<T>(self, chunked: &[Chunked<T>]) -> Result<(Vec<DebugInfoFile>, bool)>

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 wouldn't do this as a method on this trait; it seems to me that this should be a free function, although this is admittedly based on an aesthetic feeling.

Add functionality to the `ChunkOptions` trait, allowing the `ChunkServerOptions` to be obtained. With this change, `DifUpload` can no longer implement `ChunkOptions` directly, since it does not contain the server options, so we create a new `ChunkedDifUpload` struct to implement `ChunkOptions` by acting as a container for the `DifUpload` and `ChunkServerOptions`.
Also, we take advantage of the new functionality by refactoring `upload_difs_chunked` to take only a single argument, the `ChunkedDifUpload`. The upload methods now take owned options structs, since an upload should only be performed once.
The `upload_difs_chunked` function contains some code which is specific to uploading difs, and some code which is general to uploading chunked objects. This change extracts the code that is generic to chunked objects
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from 6203e7a to d923964CompareDecember 6, 2024 13:39
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-10 branch from 8caf2b4 to 73ec38eCompareDecember 6, 2024 13:40
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from d923964 to cf02c17CompareDecember 16, 2024 15:28
@szokeasaurusrex
szokeasaurusrex marked this pull request as draft December 16, 2024 15:31
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from cf02c17 to 5a78717CompareDecember 16, 2024 15:38
Base automatically changed from szokeasaurusrex/proguard-upload-9 to masterDecember 16, 2024 15:49
@szokeasaurusrex
szokeasaurusrex deleted the szokeasaurusrex/proguard-upload-10 branch December 16, 2024 15:55
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@szokeasaurusrex@loewenheim
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

ref: Extract general chunk uploading logic from upload_difs_chunked - #2303

Closed
szokeasaurusrex wants to merge 2 commits into
masterfrom
szokeasaurusrex/proguard-upload-10
Closed

ref: Extract general chunk uploading logic from upload_difs_chunked#2303
szokeasaurusrex wants to merge 2 commits into
masterfrom
szokeasaurusrex/proguard-upload-10

Conversation

@szokeasaurusrex

Copy link
Copy Markdown
Member

The upload_difs_chunked function contains some code which is specific to uploading difs, and some code which is general to uploading chunked objects. This change extracts the code that is generic to chunked objects

@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from a661c6c to 6203e7aCompareDecember 5, 2024 15:34
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-10 branch from d4736a9 to e4cd295CompareDecember 5, 2024 15:34
@szokeasaurusrexszokeasaurusrex changed the title ref: Extract general chunk uploadin logic from upload_difs_chunkedref: Extract general chunk uploading logic from upload_difs_chunkedDec 5, 2024
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-10 branch from e4cd295 to 8caf2b4CompareDecember 5, 2024 15:36
Comment on lines +336 to +346
fn render_detail(detail: &Option<String>, fallback: Option<&str>) {
let mut string = match *detail {
Some(ref string) => string.as_str(),
None => "",
};

if string.is_empty() {
if let Some(fallback) = fallback {
string = fallback;
}
}

@loewenheimloewenheimDec 5, 2024

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.

  1. &Option<…> in a function signature is kind of a code smell; you can always do better by taking Option<&…> instead. You just have to call it with .as_ref(), or in this case .as_deref().
  2. The logic of picking the detail or the fallback can be expressed more clearly.
Suggested change
fn render_detail(detail:&Option<String>,fallback:Option<&str>){
letmut string = match*detail {
Some(ref string) => string.as_str(),
None => "",
};
if string.is_empty(){
ifletSome(fallback) = fallback {
string = fallback;
}
}
fn render_detail(detail:Option<&str>,fallback:Option<&str>){
let detail = detail.unwrap_or_default();
let fallback = fallback.unwrap_or_default();
let string = if detail.is_empty(){ fallback }else{ detail };

To expand on (1) a bit: Option<&T> is strictly more general than &Option<T>; there is nothing you can do with the latter that you couldn't do with the former, but &Option<T> requires you to actually have an Option<T> somewhere that you can take a reference of.

Similarly, there's no reason to use &String over &str. &str will let you do anything that &String would, but you don't need an owned String to use it.

Combining the two, you should use Option<&str> over &Option<String>. At the call site you can use .as_deref() to get exactly the type you need.

@szokeasaurusrexszokeasaurusrexDec 6, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Fair point, but I will do this change in a separate PR. I am simply copying the existing function from dif_upload.rs in this PR

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.

Oh, I didn't realize this was existing code.

fn server_options(&self) -> &ChunkServerOptions;

/// Uploads the given chunked objects to the server.
fn upload_chunked_objects<T>(self, chunked: &[Chunked<T>]) -> Result<(Vec<DebugInfoFile>, bool)>

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 wouldn't do this as a method on this trait; it seems to me that this should be a free function, although this is admittedly based on an aesthetic feeling.

Add functionality to the `ChunkOptions` trait, allowing the `ChunkServerOptions` to be obtained. With this change, `DifUpload` can no longer implement `ChunkOptions` directly, since it does not contain the server options, so we create a new `ChunkedDifUpload` struct to implement `ChunkOptions` by acting as a container for the `DifUpload` and `ChunkServerOptions`.
Also, we take advantage of the new functionality by refactoring `upload_difs_chunked` to take only a single argument, the `ChunkedDifUpload`. The upload methods now take owned options structs, since an upload should only be performed once.
The `upload_difs_chunked` function contains some code which is specific to uploading difs, and some code which is general to uploading chunked objects. This change extracts the code that is generic to chunked objects
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from 6203e7a to d923964CompareDecember 6, 2024 13:39
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-10 branch from 8caf2b4 to 73ec38eCompareDecember 6, 2024 13:40
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from d923964 to cf02c17CompareDecember 16, 2024 15:28
@szokeasaurusrex
szokeasaurusrex marked this pull request as draft December 16, 2024 15:31
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from cf02c17 to 5a78717CompareDecember 16, 2024 15:38
Base automatically changed from szokeasaurusrex/proguard-upload-9 to masterDecember 16, 2024 15:49
@szokeasaurusrex
szokeasaurusrex deleted the szokeasaurusrex/proguard-upload-10 branch December 16, 2024 15:55
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

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

ref: Extract general chunk uploading logic from upload_difs_chunked - #2303

Closed
szokeasaurusrex wants to merge 2 commits into
masterfrom
szokeasaurusrex/proguard-upload-10
Closed

ref: Extract general chunk uploading logic from upload_difs_chunked#2303
szokeasaurusrex wants to merge 2 commits into
masterfrom
szokeasaurusrex/proguard-upload-10

Conversation

@szokeasaurusrex

Copy link
Copy Markdown
Member

The upload_difs_chunked function contains some code which is specific to uploading difs, and some code which is general to uploading chunked objects. This change extracts the code that is generic to chunked objects

@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from a661c6c to 6203e7aCompareDecember 5, 2024 15:34
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-10 branch from d4736a9 to e4cd295CompareDecember 5, 2024 15:34
@szokeasaurusrexszokeasaurusrex changed the title ref: Extract general chunk uploadin logic from upload_difs_chunkedref: Extract general chunk uploading logic from upload_difs_chunkedDec 5, 2024
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-10 branch from e4cd295 to 8caf2b4CompareDecember 5, 2024 15:36
Comment on lines +336 to +346
fn render_detail(detail: &Option<String>, fallback: Option<&str>) {
let mut string = match *detail {
Some(ref string) => string.as_str(),
None => "",
};

if string.is_empty() {
if let Some(fallback) = fallback {
string = fallback;
}
}

@loewenheimloewenheimDec 5, 2024

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.

  1. &Option<…> in a function signature is kind of a code smell; you can always do better by taking Option<&…> instead. You just have to call it with .as_ref(), or in this case .as_deref().
  2. The logic of picking the detail or the fallback can be expressed more clearly.
Suggested change
fn render_detail(detail:&Option<String>,fallback:Option<&str>){
letmut string = match*detail {
Some(ref string) => string.as_str(),
None => "",
};
if string.is_empty(){
ifletSome(fallback) = fallback {
string = fallback;
}
}
fn render_detail(detail:Option<&str>,fallback:Option<&str>){
let detail = detail.unwrap_or_default();
let fallback = fallback.unwrap_or_default();
let string = if detail.is_empty(){ fallback }else{ detail };

To expand on (1) a bit: Option<&T> is strictly more general than &Option<T>; there is nothing you can do with the latter that you couldn't do with the former, but &Option<T> requires you to actually have an Option<T> somewhere that you can take a reference of.

Similarly, there's no reason to use &String over &str. &str will let you do anything that &String would, but you don't need an owned String to use it.

Combining the two, you should use Option<&str> over &Option<String>. At the call site you can use .as_deref() to get exactly the type you need.

@szokeasaurusrexszokeasaurusrexDec 6, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Fair point, but I will do this change in a separate PR. I am simply copying the existing function from dif_upload.rs in this PR

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.

Oh, I didn't realize this was existing code.

fn server_options(&self) -> &ChunkServerOptions;

/// Uploads the given chunked objects to the server.
fn upload_chunked_objects<T>(self, chunked: &[Chunked<T>]) -> Result<(Vec<DebugInfoFile>, bool)>

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 wouldn't do this as a method on this trait; it seems to me that this should be a free function, although this is admittedly based on an aesthetic feeling.

Add functionality to the `ChunkOptions` trait, allowing the `ChunkServerOptions` to be obtained. With this change, `DifUpload` can no longer implement `ChunkOptions` directly, since it does not contain the server options, so we create a new `ChunkedDifUpload` struct to implement `ChunkOptions` by acting as a container for the `DifUpload` and `ChunkServerOptions`.
Also, we take advantage of the new functionality by refactoring `upload_difs_chunked` to take only a single argument, the `ChunkedDifUpload`. The upload methods now take owned options structs, since an upload should only be performed once.
The `upload_difs_chunked` function contains some code which is specific to uploading difs, and some code which is general to uploading chunked objects. This change extracts the code that is generic to chunked objects
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from 6203e7a to d923964CompareDecember 6, 2024 13:39
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-10 branch from 8caf2b4 to 73ec38eCompareDecember 6, 2024 13:40
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from d923964 to cf02c17CompareDecember 16, 2024 15:28
@szokeasaurusrex
szokeasaurusrex marked this pull request as draft December 16, 2024 15:31
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from cf02c17 to 5a78717CompareDecember 16, 2024 15:38
Base automatically changed from szokeasaurusrex/proguard-upload-9 to masterDecember 16, 2024 15:49
@szokeasaurusrex
szokeasaurusrex deleted the szokeasaurusrex/proguard-upload-10 branch December 16, 2024 15:55
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

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

ref: Extract general chunk uploading logic from upload_difs_chunked - #2303

Closed
szokeasaurusrex wants to merge 2 commits into
masterfrom
szokeasaurusrex/proguard-upload-10
Closed

ref: Extract general chunk uploading logic from upload_difs_chunked#2303
szokeasaurusrex wants to merge 2 commits into
masterfrom
szokeasaurusrex/proguard-upload-10

Conversation

@szokeasaurusrex

Copy link
Copy Markdown
Member

The upload_difs_chunked function contains some code which is specific to uploading difs, and some code which is general to uploading chunked objects. This change extracts the code that is generic to chunked objects

@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from a661c6c to 6203e7aCompareDecember 5, 2024 15:34
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-10 branch from d4736a9 to e4cd295CompareDecember 5, 2024 15:34
@szokeasaurusrexszokeasaurusrex changed the title ref: Extract general chunk uploadin logic from upload_difs_chunkedref: Extract general chunk uploading logic from upload_difs_chunkedDec 5, 2024
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-10 branch from e4cd295 to 8caf2b4CompareDecember 5, 2024 15:36
Comment on lines +336 to +346
fn render_detail(detail: &Option<String>, fallback: Option<&str>) {
let mut string = match *detail {
Some(ref string) => string.as_str(),
None => "",
};

if string.is_empty() {
if let Some(fallback) = fallback {
string = fallback;
}
}

@loewenheimloewenheimDec 5, 2024

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.

  1. &Option<…> in a function signature is kind of a code smell; you can always do better by taking Option<&…> instead. You just have to call it with .as_ref(), or in this case .as_deref().
  2. The logic of picking the detail or the fallback can be expressed more clearly.
Suggested change
fn render_detail(detail:&Option<String>,fallback:Option<&str>){
letmut string = match*detail {
Some(ref string) => string.as_str(),
None => "",
};
if string.is_empty(){
ifletSome(fallback) = fallback {
string = fallback;
}
}
fn render_detail(detail:Option<&str>,fallback:Option<&str>){
let detail = detail.unwrap_or_default();
let fallback = fallback.unwrap_or_default();
let string = if detail.is_empty(){ fallback }else{ detail };

To expand on (1) a bit: Option<&T> is strictly more general than &Option<T>; there is nothing you can do with the latter that you couldn't do with the former, but &Option<T> requires you to actually have an Option<T> somewhere that you can take a reference of.

Similarly, there's no reason to use &String over &str. &str will let you do anything that &String would, but you don't need an owned String to use it.

Combining the two, you should use Option<&str> over &Option<String>. At the call site you can use .as_deref() to get exactly the type you need.

@szokeasaurusrexszokeasaurusrexDec 6, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Fair point, but I will do this change in a separate PR. I am simply copying the existing function from dif_upload.rs in this PR

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.

Oh, I didn't realize this was existing code.

fn server_options(&self) -> &ChunkServerOptions;

/// Uploads the given chunked objects to the server.
fn upload_chunked_objects<T>(self, chunked: &[Chunked<T>]) -> Result<(Vec<DebugInfoFile>, bool)>

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 wouldn't do this as a method on this trait; it seems to me that this should be a free function, although this is admittedly based on an aesthetic feeling.

Add functionality to the `ChunkOptions` trait, allowing the `ChunkServerOptions` to be obtained. With this change, `DifUpload` can no longer implement `ChunkOptions` directly, since it does not contain the server options, so we create a new `ChunkedDifUpload` struct to implement `ChunkOptions` by acting as a container for the `DifUpload` and `ChunkServerOptions`.
Also, we take advantage of the new functionality by refactoring `upload_difs_chunked` to take only a single argument, the `ChunkedDifUpload`. The upload methods now take owned options structs, since an upload should only be performed once.
The `upload_difs_chunked` function contains some code which is specific to uploading difs, and some code which is general to uploading chunked objects. This change extracts the code that is generic to chunked objects
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from 6203e7a to d923964CompareDecember 6, 2024 13:39
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-10 branch from 8caf2b4 to 73ec38eCompareDecember 6, 2024 13:40
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from d923964 to cf02c17CompareDecember 16, 2024 15:28
@szokeasaurusrex
szokeasaurusrex marked this pull request as draft December 16, 2024 15:31
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from cf02c17 to 5a78717CompareDecember 16, 2024 15:38
Base automatically changed from szokeasaurusrex/proguard-upload-9 to masterDecember 16, 2024 15:49
@szokeasaurusrex
szokeasaurusrex deleted the szokeasaurusrex/proguard-upload-10 branch December 16, 2024 15:55
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@szokeasaurusrex@loewenheim
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

ref: Extract general chunk uploading logic from upload_difs_chunked - #2303

Closed
szokeasaurusrex wants to merge 2 commits into
masterfrom
szokeasaurusrex/proguard-upload-10
Closed

ref: Extract general chunk uploading logic from upload_difs_chunked#2303
szokeasaurusrex wants to merge 2 commits into
masterfrom
szokeasaurusrex/proguard-upload-10

Conversation

@szokeasaurusrex

Copy link
Copy Markdown
Member

The upload_difs_chunked function contains some code which is specific to uploading difs, and some code which is general to uploading chunked objects. This change extracts the code that is generic to chunked objects

@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from a661c6c to 6203e7aCompareDecember 5, 2024 15:34
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-10 branch from d4736a9 to e4cd295CompareDecember 5, 2024 15:34
@szokeasaurusrexszokeasaurusrex changed the title ref: Extract general chunk uploadin logic from upload_difs_chunkedref: Extract general chunk uploading logic from upload_difs_chunkedDec 5, 2024
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-10 branch from e4cd295 to 8caf2b4CompareDecember 5, 2024 15:36
Comment on lines +336 to +346
fn render_detail(detail: &Option<String>, fallback: Option<&str>) {
let mut string = match *detail {
Some(ref string) => string.as_str(),
None => "",
};

if string.is_empty() {
if let Some(fallback) = fallback {
string = fallback;
}
}

@loewenheimloewenheimDec 5, 2024

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.

  1. &Option<…> in a function signature is kind of a code smell; you can always do better by taking Option<&…> instead. You just have to call it with .as_ref(), or in this case .as_deref().
  2. The logic of picking the detail or the fallback can be expressed more clearly.
Suggested change
fn render_detail(detail:&Option<String>,fallback:Option<&str>){
letmut string = match*detail {
Some(ref string) => string.as_str(),
None => "",
};
if string.is_empty(){
ifletSome(fallback) = fallback {
string = fallback;
}
}
fn render_detail(detail:Option<&str>,fallback:Option<&str>){
let detail = detail.unwrap_or_default();
let fallback = fallback.unwrap_or_default();
let string = if detail.is_empty(){ fallback }else{ detail };

To expand on (1) a bit: Option<&T> is strictly more general than &Option<T>; there is nothing you can do with the latter that you couldn't do with the former, but &Option<T> requires you to actually have an Option<T> somewhere that you can take a reference of.

Similarly, there's no reason to use &String over &str. &str will let you do anything that &String would, but you don't need an owned String to use it.

Combining the two, you should use Option<&str> over &Option<String>. At the call site you can use .as_deref() to get exactly the type you need.

@szokeasaurusrexszokeasaurusrexDec 6, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Fair point, but I will do this change in a separate PR. I am simply copying the existing function from dif_upload.rs in this PR

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.

Oh, I didn't realize this was existing code.

fn server_options(&self) -> &ChunkServerOptions;

/// Uploads the given chunked objects to the server.
fn upload_chunked_objects<T>(self, chunked: &[Chunked<T>]) -> Result<(Vec<DebugInfoFile>, bool)>

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 wouldn't do this as a method on this trait; it seems to me that this should be a free function, although this is admittedly based on an aesthetic feeling.

Add functionality to the `ChunkOptions` trait, allowing the `ChunkServerOptions` to be obtained. With this change, `DifUpload` can no longer implement `ChunkOptions` directly, since it does not contain the server options, so we create a new `ChunkedDifUpload` struct to implement `ChunkOptions` by acting as a container for the `DifUpload` and `ChunkServerOptions`.
Also, we take advantage of the new functionality by refactoring `upload_difs_chunked` to take only a single argument, the `ChunkedDifUpload`. The upload methods now take owned options structs, since an upload should only be performed once.
The `upload_difs_chunked` function contains some code which is specific to uploading difs, and some code which is general to uploading chunked objects. This change extracts the code that is generic to chunked objects
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from 6203e7a to d923964CompareDecember 6, 2024 13:39
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-10 branch from 8caf2b4 to 73ec38eCompareDecember 6, 2024 13:40
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from d923964 to cf02c17CompareDecember 16, 2024 15:28
@szokeasaurusrex
szokeasaurusrex marked this pull request as draft December 16, 2024 15:31
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from cf02c17 to 5a78717CompareDecember 16, 2024 15:38
Base automatically changed from szokeasaurusrex/proguard-upload-9 to masterDecember 16, 2024 15:49
@szokeasaurusrex
szokeasaurusrex deleted the szokeasaurusrex/proguard-upload-10 branch December 16, 2024 15:55
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@szokeasaurusrex@loewenheim
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

ref: Extract general chunk uploading logic from upload_difs_chunked - #2303

Closed
szokeasaurusrex wants to merge 2 commits into
masterfrom
szokeasaurusrex/proguard-upload-10
Closed

ref: Extract general chunk uploading logic from upload_difs_chunked#2303
szokeasaurusrex wants to merge 2 commits into
masterfrom
szokeasaurusrex/proguard-upload-10

Conversation

@szokeasaurusrex

Copy link
Copy Markdown
Member

The upload_difs_chunked function contains some code which is specific to uploading difs, and some code which is general to uploading chunked objects. This change extracts the code that is generic to chunked objects

@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from a661c6c to 6203e7aCompareDecember 5, 2024 15:34
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-10 branch from d4736a9 to e4cd295CompareDecember 5, 2024 15:34
@szokeasaurusrexszokeasaurusrex changed the title ref: Extract general chunk uploadin logic from upload_difs_chunkedref: Extract general chunk uploading logic from upload_difs_chunkedDec 5, 2024
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-10 branch from e4cd295 to 8caf2b4CompareDecember 5, 2024 15:36
Comment on lines +336 to +346
fn render_detail(detail: &Option<String>, fallback: Option<&str>) {
let mut string = match *detail {
Some(ref string) => string.as_str(),
None => "",
};

if string.is_empty() {
if let Some(fallback) = fallback {
string = fallback;
}
}

@loewenheimloewenheimDec 5, 2024

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.

  1. &Option<…> in a function signature is kind of a code smell; you can always do better by taking Option<&…> instead. You just have to call it with .as_ref(), or in this case .as_deref().
  2. The logic of picking the detail or the fallback can be expressed more clearly.
Suggested change
fn render_detail(detail:&Option<String>,fallback:Option<&str>){
letmut string = match*detail {
Some(ref string) => string.as_str(),
None => "",
};
if string.is_empty(){
ifletSome(fallback) = fallback {
string = fallback;
}
}
fn render_detail(detail:Option<&str>,fallback:Option<&str>){
let detail = detail.unwrap_or_default();
let fallback = fallback.unwrap_or_default();
let string = if detail.is_empty(){ fallback }else{ detail };

To expand on (1) a bit: Option<&T> is strictly more general than &Option<T>; there is nothing you can do with the latter that you couldn't do with the former, but &Option<T> requires you to actually have an Option<T> somewhere that you can take a reference of.

Similarly, there's no reason to use &String over &str. &str will let you do anything that &String would, but you don't need an owned String to use it.

Combining the two, you should use Option<&str> over &Option<String>. At the call site you can use .as_deref() to get exactly the type you need.

@szokeasaurusrexszokeasaurusrexDec 6, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Fair point, but I will do this change in a separate PR. I am simply copying the existing function from dif_upload.rs in this PR

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.

Oh, I didn't realize this was existing code.

fn server_options(&self) -> &ChunkServerOptions;

/// Uploads the given chunked objects to the server.
fn upload_chunked_objects<T>(self, chunked: &[Chunked<T>]) -> Result<(Vec<DebugInfoFile>, bool)>

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 wouldn't do this as a method on this trait; it seems to me that this should be a free function, although this is admittedly based on an aesthetic feeling.

Add functionality to the `ChunkOptions` trait, allowing the `ChunkServerOptions` to be obtained. With this change, `DifUpload` can no longer implement `ChunkOptions` directly, since it does not contain the server options, so we create a new `ChunkedDifUpload` struct to implement `ChunkOptions` by acting as a container for the `DifUpload` and `ChunkServerOptions`.
Also, we take advantage of the new functionality by refactoring `upload_difs_chunked` to take only a single argument, the `ChunkedDifUpload`. The upload methods now take owned options structs, since an upload should only be performed once.
The `upload_difs_chunked` function contains some code which is specific to uploading difs, and some code which is general to uploading chunked objects. This change extracts the code that is generic to chunked objects
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from 6203e7a to d923964CompareDecember 6, 2024 13:39
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-10 branch from 8caf2b4 to 73ec38eCompareDecember 6, 2024 13:40
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from d923964 to cf02c17CompareDecember 16, 2024 15:28
@szokeasaurusrex
szokeasaurusrex marked this pull request as draft December 16, 2024 15:31
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from cf02c17 to 5a78717CompareDecember 16, 2024 15:38
Base automatically changed from szokeasaurusrex/proguard-upload-9 to masterDecember 16, 2024 15:49
@szokeasaurusrex
szokeasaurusrex deleted the szokeasaurusrex/proguard-upload-10 branch December 16, 2024 15:55
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@szokeasaurusrex@loewenheim
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

ref: Extract general chunk uploading logic from upload_difs_chunked - #2303

Closed
szokeasaurusrex wants to merge 2 commits into
masterfrom
szokeasaurusrex/proguard-upload-10
Closed

ref: Extract general chunk uploading logic from upload_difs_chunked#2303
szokeasaurusrex wants to merge 2 commits into
masterfrom
szokeasaurusrex/proguard-upload-10

Conversation

@szokeasaurusrex

Copy link
Copy Markdown
Member

The upload_difs_chunked function contains some code which is specific to uploading difs, and some code which is general to uploading chunked objects. This change extracts the code that is generic to chunked objects

@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from a661c6c to 6203e7aCompareDecember 5, 2024 15:34
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-10 branch from d4736a9 to e4cd295CompareDecember 5, 2024 15:34
@szokeasaurusrexszokeasaurusrex changed the title ref: Extract general chunk uploadin logic from upload_difs_chunkedref: Extract general chunk uploading logic from upload_difs_chunkedDec 5, 2024
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-10 branch from e4cd295 to 8caf2b4CompareDecember 5, 2024 15:36
Comment on lines +336 to +346
fn render_detail(detail: &Option<String>, fallback: Option<&str>) {
let mut string = match *detail {
Some(ref string) => string.as_str(),
None => "",
};

if string.is_empty() {
if let Some(fallback) = fallback {
string = fallback;
}
}

@loewenheimloewenheimDec 5, 2024

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.

  1. &Option<…> in a function signature is kind of a code smell; you can always do better by taking Option<&…> instead. You just have to call it with .as_ref(), or in this case .as_deref().
  2. The logic of picking the detail or the fallback can be expressed more clearly.
Suggested change
fn render_detail(detail:&Option<String>,fallback:Option<&str>){
letmut string = match*detail {
Some(ref string) => string.as_str(),
None => "",
};
if string.is_empty(){
ifletSome(fallback) = fallback {
string = fallback;
}
}
fn render_detail(detail:Option<&str>,fallback:Option<&str>){
let detail = detail.unwrap_or_default();
let fallback = fallback.unwrap_or_default();
let string = if detail.is_empty(){ fallback }else{ detail };

To expand on (1) a bit: Option<&T> is strictly more general than &Option<T>; there is nothing you can do with the latter that you couldn't do with the former, but &Option<T> requires you to actually have an Option<T> somewhere that you can take a reference of.

Similarly, there's no reason to use &String over &str. &str will let you do anything that &String would, but you don't need an owned String to use it.

Combining the two, you should use Option<&str> over &Option<String>. At the call site you can use .as_deref() to get exactly the type you need.

@szokeasaurusrexszokeasaurusrexDec 6, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Fair point, but I will do this change in a separate PR. I am simply copying the existing function from dif_upload.rs in this PR

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.

Oh, I didn't realize this was existing code.

fn server_options(&self) -> &ChunkServerOptions;

/// Uploads the given chunked objects to the server.
fn upload_chunked_objects<T>(self, chunked: &[Chunked<T>]) -> Result<(Vec<DebugInfoFile>, bool)>

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 wouldn't do this as a method on this trait; it seems to me that this should be a free function, although this is admittedly based on an aesthetic feeling.

Add functionality to the `ChunkOptions` trait, allowing the `ChunkServerOptions` to be obtained. With this change, `DifUpload` can no longer implement `ChunkOptions` directly, since it does not contain the server options, so we create a new `ChunkedDifUpload` struct to implement `ChunkOptions` by acting as a container for the `DifUpload` and `ChunkServerOptions`.
Also, we take advantage of the new functionality by refactoring `upload_difs_chunked` to take only a single argument, the `ChunkedDifUpload`. The upload methods now take owned options structs, since an upload should only be performed once.
The `upload_difs_chunked` function contains some code which is specific to uploading difs, and some code which is general to uploading chunked objects. This change extracts the code that is generic to chunked objects
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from 6203e7a to d923964CompareDecember 6, 2024 13:39
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-10 branch from 8caf2b4 to 73ec38eCompareDecember 6, 2024 13:40
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from d923964 to cf02c17CompareDecember 16, 2024 15:28
@szokeasaurusrex
szokeasaurusrex marked this pull request as draft December 16, 2024 15:31
@szokeasaurusrex
szokeasaurusrexforce-pushed the szokeasaurusrex/proguard-upload-9 branch from cf02c17 to 5a78717CompareDecember 16, 2024 15:38
Base automatically changed from szokeasaurusrex/proguard-upload-9 to masterDecember 16, 2024 15:49
@szokeasaurusrex
szokeasaurusrex deleted the szokeasaurusrex/proguard-upload-10 branch December 16, 2024 15:55
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@szokeasaurusrex@loewenheim