make toMap() json compatible - #1132

Closed
Yczar wants to merge 5 commits into
fluttercommunity:mainfrom
Yczar:main
Closed

make toMap() json compatible#1132
Yczar wants to merge 5 commits into
fluttercommunity:mainfrom
Yczar:main

Conversation

@Yczar

@YczarYczar commented Oct 2, 2022•
edited
Loading

Copy link
Copy Markdown

Description

Rendered toMap on the models to be jsonCompatible

Related Issues

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated the version in pubspec.yaml and CHANGELOG.md.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@YczarYczar mentioned this pull request Oct 2, 2022
Closed
@miquelbeltran

miquelbeltran commented Oct 2, 2022

Copy link
Copy Markdown
Member

An @overrides is missing, check the analyze output.

Would it be possible to add unit tests for the toJson methods?

@Yczar

Yczar commented Oct 2, 2022

Copy link
Copy Markdown
Author

An @overrides is missing, check the analyze output.

Would it be possible to add unit tests for the toJson methods?

Fixed these on the latest push @miquelbeltran

@iapicca

iapicca commented Oct 2, 2022

Copy link
Copy Markdown
Contributor

@miquelbeltran
shouldn't "toJson" return a json rather than a string?

@Yczar

Yczar commented Oct 2, 2022

Copy link
Copy Markdown
Author

@miquelbeltran shouldn't "toJson" return a JSON rather than a string?

toJson should return a parseable string otherwise it is just toMap with a change of name. @iapicca

@iapiccaiapicca mentioned this pull request Oct 2, 2022
Closed
8 tasks
@iapicca

iapicca commented Oct 2, 2022

Copy link
Copy Markdown
Contributor

@miquelbeltran shouldn't "toJson" return a JSON rather than a string?

toJson should return a parseable string otherwise it is just toMap with a change of name. @iapicca

I'm saying that this PR doesn't the issue you linked
the current problem is that toMap breaks dart:convert as it returns an enum somewhere

I fixed in this PR,
but I didn't get any answer in 2 months

your approach seems to be adding a toJson method rather than fixing toMap
similar approach as been previously described as not needed by @vbuberen

and quoting@miquelbeltran

I am sorry, I am not sure anymore if we need this. If a user wants to serialize the contents of a DeviceInfo object, they should do it on their own app data model. We shouldn't be maintaining a toJson implementation on our side, even if convenient, it is not our job. Like, I am not even sure why are we providing a toMap method even, I guess that's the legacy from when we got the plugins from Google.

@Yczar

Yczar commented Oct 2, 2022

Copy link
Copy Markdown
Author

@miquelbeltran shouldn't "toJson" return a JSON rather than a string?

toJson should return a parseable string otherwise it is just toMap with a change of name. @iapicca

I'm saying that this PR doesn't the issue you linked the current problem is that toMap breaks dart:convert as it returns an enum somewhere

I fixed in this PR, but I didn't get any answer in 2 months

your approach seems to be adding a toJson method rather than fixing toMap similar approach as been previously described as not needed by @vbuberen

and quoting@miquelbeltran

I am sorry, I am not sure anymore if we need this. If a user wants to serialize the contents of a DeviceInfo object, they should do it on their own app data model. We shouldn't be maintaining a toJson implementation on our side, even if convenient, it is not our job. Like, I am not even sure why are we providing a toMap method even, I guess that's the legacy from when we got the plugins from Google.

All checks passed on this end... I feel we should leave it to the regulators to decide which approach is right or wrong... I noticed the enum was redundant Because there is already a function and a getter that generates the enum in the first place. So there was never a need for the breaks to happen. But if the maintainers find your approach interesting then I believe they would do the justice.

@miquelbeltran

miquelbeltran commented Oct 2, 2022

Copy link
Copy Markdown
Member

@iapicca sorry, I forgot that we had your PR on this, I apologize for not having followed up.

Since the original issue has been closed, I will simply close this PR as well.

As I though originally, I am not in favor of adding extra functionality to the plugins that should be external to the plugin, like JSON support.

@vbuberen

vbuberen commented Oct 2, 2022

Copy link
Copy Markdown
Collaborator

similar approach as been previously described as not needed by @vbuberen

Would be good if you could calm down a little bit first, because seems like the miscommunication issue and lack of response from the reviewer of your PR made you feel offended.

Secondly, I wasn't stating anything there, but was genuinely curios to find out what is the reason behind your changes. So would be also good to understand that such question might have a different meaning than what you implied.

@github-actionsgithub-actionsBot locked as resolved and limited conversation to collaborators Jan 31, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Request]: make toMap() json compatible

4 participants

@Yczar@miquelbeltran@iapicca@vbuberen
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 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
make toMap() json compatible - #1132
Closed
Yczar wants to merge 5 commits into
fluttercommunity:mainfrom 
Yczar:main
Closed
make toMap() json compatible#1132
Yczar wants to merge 5 commits into
fluttercommunity:mainfrom 
Yczar:main
Conversation
@Yczar
@YczarYczar
commented
Oct 2, 2022•

edited

Loading

Copy link


Copy Markdown

Description
Rendered toMap on the models to be jsonCompatible
Related Issues
Checklist
  •  I read the Contributor Guide and followed the process outlined there for submitting PRs.
  •  My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  •  All existing and new tests are passing.
  •  I updated the version in pubspec.yaml and CHANGELOG.md.
  •  I updated/added relevant documentation (doc comments with ///).
  •  The analyzer (flutter analyze) does not report any problems on my PR.
  •  I read and followed the Flutter Style Guide.
  •  I am willing to follow-up on review comments in a timely manner.
Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?
  •  Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  •  No, this is not a breaking change.
@YczarYczar
mentioned this pull request
Oct 2, 2022

Closed

@miquelbeltran
miquelbeltran
commented
Oct 2, 2022

Copy link


Copy Markdown

Member
An @overrides is missing, check the analyze output.
Would it be possible to add unit tests for the toJson methods?
@Yczar
Yczar
commented
Oct 2, 2022

Copy link


Copy Markdown

Author
An @overrides is missing, check the analyze output.
Would it be possible to add unit tests for the toJson methods?
Fixed these on the latest push @miquelbeltran
@iapicca
iapicca
commented
Oct 2, 2022

Copy link


Copy Markdown

Contributor
@miquelbeltran

shouldn't "toJson" return a json rather than a string?
@Yczar
Yczar
commented
Oct 2, 2022

Copy link


Copy Markdown

Author
@miquelbeltran shouldn't "toJson" return a JSON rather than a string?
toJson should return a parseable string otherwise it is just toMap with a change of name. @iapicca
@iapiccaiapicca
mentioned this pull request
Oct 2, 2022
 Closed

8 tasks
@iapicca
iapicca
commented
Oct 2, 2022

Copy link


Copy Markdown

Contributor
@miquelbeltran shouldn't "toJson" return a JSON rather than a string?
toJson should return a parseable string otherwise it is just toMap with a change of name. @iapicca
I'm saying that this PR doesn't the issue you linked

the current problem is that toMap breaks dart:convert as it returns an enum somewhere
I fixed in this PR,

but I didn't get any answer in 2 months
your approach seems to be adding a toJson method rather than fixing toMap

similar approach as been previously described as not needed by @vbuberen
and quoting@miquelbeltran
I am sorry, I am not sure anymore if we need this. If a user wants to serialize the contents of a DeviceInfo object, they should do it on their own app data model. We shouldn't be maintaining a toJson implementation on our side, even if convenient, it is not our job. Like, I am not even sure why are we providing a toMap method even, I guess that's the legacy from when we got the plugins from Google.
@Yczar
Yczar
commented
Oct 2, 2022

Copy link


Copy Markdown

Author
@miquelbeltran shouldn't "toJson" return a JSON rather than a string?
toJson should return a parseable string otherwise it is just toMap with a change of name. @iapicca
I'm saying that this PR doesn't the issue you linked the current problem is that toMap breaks dart:convert as it returns an enum somewhere
I fixed in this PR, but I didn't get any answer in 2 months
your approach seems to be adding a toJson method rather than fixing toMap similar approach as been previously described as not needed by @vbuberen
and quoting@miquelbeltran
I am sorry, I am not sure anymore if we need this. If a user wants to serialize the contents of a DeviceInfo object, they should do it on their own app data model. We shouldn't be maintaining a toJson implementation on our side, even if convenient, it is not our job. Like, I am not even sure why are we providing a toMap method even, I guess that's the legacy from when we got the plugins from Google.
All checks passed on this end... I feel we should leave it to the regulators to decide which approach is right or wrong... I noticed the enum was redundant Because there is already a function and a getter that generates the enum in the first place. So there was never a need for the breaks to happen. But if the maintainers find your approach interesting then I believe they would do the justice.
@miquelbeltran
miquelbeltran
commented
Oct 2, 2022

Copy link


Copy Markdown

Member
@iapicca sorry, I forgot that we had your PR on this, I apologize for not having followed up.
Since the original issue has been closed, I will simply close this PR as well.
As I though originally, I am not in favor of adding extra functionality to the plugins that should be external to the plugin, like JSON support.
@vbuberen
vbuberen
commented
Oct 2, 2022

Copy link


Copy Markdown

Collaborator
similar approach as been previously described as not needed by @vbuberen
Would be good if you could calm down a little bit first, because seems like the miscommunication issue and lack of response from the reviewer of your PR made you feel offended.
Secondly, I wasn't stating anything there, but was genuinely curios to find out what is the reason behind your changes. So would be also good to understand that such question might have a different meaning than what you implied.
@github-actionsgithub-actionsBot
locked as resolved and limited conversation to collaborators
Jan 31, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


No reviews


Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[Request]: make toMap() json compatible


4 participants

@Yczar@miquelbeltran@iapicca@vbuberen

, '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
make toMap() json compatible - #1132
Closed
Yczar wants to merge 5 commits into
fluttercommunity:mainfrom 
Yczar:main
Closed
make toMap() json compatible#1132
Yczar wants to merge 5 commits into
fluttercommunity:mainfrom 
Yczar:main
Conversation
@Yczar
@YczarYczar
commented
Oct 2, 2022•

edited

Loading

Copy link


Copy Markdown

Description
Rendered toMap on the models to be jsonCompatible
Related Issues
Checklist
  •  I read the Contributor Guide and followed the process outlined there for submitting PRs.
  •  My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  •  All existing and new tests are passing.
  •  I updated the version in pubspec.yaml and CHANGELOG.md.
  •  I updated/added relevant documentation (doc comments with ///).
  •  The analyzer (flutter analyze) does not report any problems on my PR.
  •  I read and followed the Flutter Style Guide.
  •  I am willing to follow-up on review comments in a timely manner.
Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?
  •  Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  •  No, this is not a breaking change.
@YczarYczar
mentioned this pull request
Oct 2, 2022

Closed

@miquelbeltran
miquelbeltran
commented
Oct 2, 2022

Copy link


Copy Markdown

Member
An @overrides is missing, check the analyze output.
Would it be possible to add unit tests for the toJson methods?
@Yczar
Yczar
commented
Oct 2, 2022

Copy link


Copy Markdown

Author
An @overrides is missing, check the analyze output.
Would it be possible to add unit tests for the toJson methods?
Fixed these on the latest push @miquelbeltran
@iapicca
iapicca
commented
Oct 2, 2022

Copy link


Copy Markdown

Contributor
@miquelbeltran

shouldn't "toJson" return a json rather than a string?
@Yczar
Yczar
commented
Oct 2, 2022

Copy link


Copy Markdown

Author
@miquelbeltran shouldn't "toJson" return a JSON rather than a string?
toJson should return a parseable string otherwise it is just toMap with a change of name. @iapicca
@iapiccaiapicca
mentioned this pull request
Oct 2, 2022
 Closed

8 tasks
@iapicca
iapicca
commented
Oct 2, 2022

Copy link


Copy Markdown

Contributor
@miquelbeltran shouldn't "toJson" return a JSON rather than a string?
toJson should return a parseable string otherwise it is just toMap with a change of name. @iapicca
I'm saying that this PR doesn't the issue you linked

the current problem is that toMap breaks dart:convert as it returns an enum somewhere
I fixed in this PR,

but I didn't get any answer in 2 months
your approach seems to be adding a toJson method rather than fixing toMap

similar approach as been previously described as not needed by @vbuberen
and quoting@miquelbeltran
I am sorry, I am not sure anymore if we need this. If a user wants to serialize the contents of a DeviceInfo object, they should do it on their own app data model. We shouldn't be maintaining a toJson implementation on our side, even if convenient, it is not our job. Like, I am not even sure why are we providing a toMap method even, I guess that's the legacy from when we got the plugins from Google.
@Yczar
Yczar
commented
Oct 2, 2022

Copy link


Copy Markdown

Author
@miquelbeltran shouldn't "toJson" return a JSON rather than a string?
toJson should return a parseable string otherwise it is just toMap with a change of name. @iapicca
I'm saying that this PR doesn't the issue you linked the current problem is that toMap breaks dart:convert as it returns an enum somewhere
I fixed in this PR, but I didn't get any answer in 2 months
your approach seems to be adding a toJson method rather than fixing toMap similar approach as been previously described as not needed by @vbuberen
and quoting@miquelbeltran
I am sorry, I am not sure anymore if we need this. If a user wants to serialize the contents of a DeviceInfo object, they should do it on their own app data model. We shouldn't be maintaining a toJson implementation on our side, even if convenient, it is not our job. Like, I am not even sure why are we providing a toMap method even, I guess that's the legacy from when we got the plugins from Google.
All checks passed on this end... I feel we should leave it to the regulators to decide which approach is right or wrong... I noticed the enum was redundant Because there is already a function and a getter that generates the enum in the first place. So there was never a need for the breaks to happen. But if the maintainers find your approach interesting then I believe they would do the justice.
@miquelbeltran
miquelbeltran
commented
Oct 2, 2022

Copy link


Copy Markdown

Member
@iapicca sorry, I forgot that we had your PR on this, I apologize for not having followed up.
Since the original issue has been closed, I will simply close this PR as well.
As I though originally, I am not in favor of adding extra functionality to the plugins that should be external to the plugin, like JSON support.
@vbuberen
vbuberen
commented
Oct 2, 2022

Copy link


Copy Markdown

Collaborator
similar approach as been previously described as not needed by @vbuberen
Would be good if you could calm down a little bit first, because seems like the miscommunication issue and lack of response from the reviewer of your PR made you feel offended.
Secondly, I wasn't stating anything there, but was genuinely curios to find out what is the reason behind your changes. So would be also good to understand that such question might have a different meaning than what you implied.
@github-actionsgithub-actionsBot
locked as resolved and limited conversation to collaborators
Jan 31, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


No reviews


Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[Request]: make toMap() json compatible


4 participants

@Yczar@miquelbeltran@iapicca@vbuberen

, '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 > 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
make toMap() json compatible - #1132
Closed
Yczar wants to merge 5 commits into
fluttercommunity:mainfrom 
Yczar:main
Closed
make toMap() json compatible#1132
Yczar wants to merge 5 commits into
fluttercommunity:mainfrom 
Yczar:main
Conversation
@Yczar
@YczarYczar
commented
Oct 2, 2022•

edited

Loading

Copy link


Copy Markdown

Description
Rendered toMap on the models to be jsonCompatible
Related Issues
Checklist
  •  I read the Contributor Guide and followed the process outlined there for submitting PRs.
  •  My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  •  All existing and new tests are passing.
  •  I updated the version in pubspec.yaml and CHANGELOG.md.
  •  I updated/added relevant documentation (doc comments with ///).
  •  The analyzer (flutter analyze) does not report any problems on my PR.
  •  I read and followed the Flutter Style Guide.
  •  I am willing to follow-up on review comments in a timely manner.
Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?
  •  Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  •  No, this is not a breaking change.
@YczarYczar
mentioned this pull request
Oct 2, 2022

Closed

@miquelbeltran
miquelbeltran
commented
Oct 2, 2022

Copy link


Copy Markdown

Member
An @overrides is missing, check the analyze output.
Would it be possible to add unit tests for the toJson methods?
@Yczar
Yczar
commented
Oct 2, 2022

Copy link


Copy Markdown

Author
An @overrides is missing, check the analyze output.
Would it be possible to add unit tests for the toJson methods?
Fixed these on the latest push @miquelbeltran
@iapicca
iapicca
commented
Oct 2, 2022

Copy link


Copy Markdown

Contributor
@miquelbeltran

shouldn't "toJson" return a json rather than a string?
@Yczar
Yczar
commented
Oct 2, 2022

Copy link


Copy Markdown

Author
@miquelbeltran shouldn't "toJson" return a JSON rather than a string?
toJson should return a parseable string otherwise it is just toMap with a change of name. @iapicca
@iapiccaiapicca
mentioned this pull request
Oct 2, 2022
 Closed

8 tasks
@iapicca
iapicca
commented
Oct 2, 2022

Copy link


Copy Markdown

Contributor
@miquelbeltran shouldn't "toJson" return a JSON rather than a string?
toJson should return a parseable string otherwise it is just toMap with a change of name. @iapicca
I'm saying that this PR doesn't the issue you linked

the current problem is that toMap breaks dart:convert as it returns an enum somewhere
I fixed in this PR,

but I didn't get any answer in 2 months
your approach seems to be adding a toJson method rather than fixing toMap

similar approach as been previously described as not needed by @vbuberen
and quoting@miquelbeltran
I am sorry, I am not sure anymore if we need this. If a user wants to serialize the contents of a DeviceInfo object, they should do it on their own app data model. We shouldn't be maintaining a toJson implementation on our side, even if convenient, it is not our job. Like, I am not even sure why are we providing a toMap method even, I guess that's the legacy from when we got the plugins from Google.
@Yczar
Yczar
commented
Oct 2, 2022

Copy link


Copy Markdown

Author
@miquelbeltran shouldn't "toJson" return a JSON rather than a string?
toJson should return a parseable string otherwise it is just toMap with a change of name. @iapicca
I'm saying that this PR doesn't the issue you linked the current problem is that toMap breaks dart:convert as it returns an enum somewhere
I fixed in this PR, but I didn't get any answer in 2 months
your approach seems to be adding a toJson method rather than fixing toMap similar approach as been previously described as not needed by @vbuberen
and quoting@miquelbeltran
I am sorry, I am not sure anymore if we need this. If a user wants to serialize the contents of a DeviceInfo object, they should do it on their own app data model. We shouldn't be maintaining a toJson implementation on our side, even if convenient, it is not our job. Like, I am not even sure why are we providing a toMap method even, I guess that's the legacy from when we got the plugins from Google.
All checks passed on this end... I feel we should leave it to the regulators to decide which approach is right or wrong... I noticed the enum was redundant Because there is already a function and a getter that generates the enum in the first place. So there was never a need for the breaks to happen. But if the maintainers find your approach interesting then I believe they would do the justice.
@miquelbeltran
miquelbeltran
commented
Oct 2, 2022

Copy link


Copy Markdown

Member
@iapicca sorry, I forgot that we had your PR on this, I apologize for not having followed up.
Since the original issue has been closed, I will simply close this PR as well.
As I though originally, I am not in favor of adding extra functionality to the plugins that should be external to the plugin, like JSON support.
@vbuberen
vbuberen
commented
Oct 2, 2022

Copy link


Copy Markdown

Collaborator
similar approach as been previously described as not needed by @vbuberen
Would be good if you could calm down a little bit first, because seems like the miscommunication issue and lack of response from the reviewer of your PR made you feel offended.
Secondly, I wasn't stating anything there, but was genuinely curios to find out what is the reason behind your changes. So would be also good to understand that such question might have a different meaning than what you implied.
@github-actionsgithub-actionsBot
locked as resolved and limited conversation to collaborators
Jan 31, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


No reviews


Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[Request]: make toMap() json compatible


4 participants

@Yczar@miquelbeltran@iapicca@vbuberen

, '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
make toMap() json compatible - #1132
Closed
Yczar wants to merge 5 commits into
fluttercommunity:mainfrom 
Yczar:main
Closed
make toMap() json compatible#1132
Yczar wants to merge 5 commits into
fluttercommunity:mainfrom 
Yczar:main
Conversation
@Yczar
@YczarYczar
commented
Oct 2, 2022•

edited

Loading

Copy link


Copy Markdown

Description
Rendered toMap on the models to be jsonCompatible
Related Issues
Checklist
  •  I read the Contributor Guide and followed the process outlined there for submitting PRs.
  •  My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  •  All existing and new tests are passing.
  •  I updated the version in pubspec.yaml and CHANGELOG.md.
  •  I updated/added relevant documentation (doc comments with ///).
  •  The analyzer (flutter analyze) does not report any problems on my PR.
  •  I read and followed the Flutter Style Guide.
  •  I am willing to follow-up on review comments in a timely manner.
Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?
  •  Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  •  No, this is not a breaking change.
@YczarYczar
mentioned this pull request
Oct 2, 2022

Closed

@miquelbeltran
miquelbeltran
commented
Oct 2, 2022

Copy link


Copy Markdown

Member
An @overrides is missing, check the analyze output.
Would it be possible to add unit tests for the toJson methods?
@Yczar
Yczar
commented
Oct 2, 2022

Copy link


Copy Markdown

Author
An @overrides is missing, check the analyze output.
Would it be possible to add unit tests for the toJson methods?
Fixed these on the latest push @miquelbeltran
@iapicca
iapicca
commented
Oct 2, 2022

Copy link


Copy Markdown

Contributor
@miquelbeltran

shouldn't "toJson" return a json rather than a string?
@Yczar
Yczar
commented
Oct 2, 2022

Copy link


Copy Markdown

Author
@miquelbeltran shouldn't "toJson" return a JSON rather than a string?
toJson should return a parseable string otherwise it is just toMap with a change of name. @iapicca
@iapiccaiapicca
mentioned this pull request
Oct 2, 2022
 Closed

8 tasks
@iapicca
iapicca
commented
Oct 2, 2022

Copy link


Copy Markdown

Contributor
@miquelbeltran shouldn't "toJson" return a JSON rather than a string?
toJson should return a parseable string otherwise it is just toMap with a change of name. @iapicca
I'm saying that this PR doesn't the issue you linked

the current problem is that toMap breaks dart:convert as it returns an enum somewhere
I fixed in this PR,

but I didn't get any answer in 2 months
your approach seems to be adding a toJson method rather than fixing toMap

similar approach as been previously described as not needed by @vbuberen
and quoting@miquelbeltran
I am sorry, I am not sure anymore if we need this. If a user wants to serialize the contents of a DeviceInfo object, they should do it on their own app data model. We shouldn't be maintaining a toJson implementation on our side, even if convenient, it is not our job. Like, I am not even sure why are we providing a toMap method even, I guess that's the legacy from when we got the plugins from Google.
@Yczar
Yczar
commented
Oct 2, 2022

Copy link


Copy Markdown

Author
@miquelbeltran shouldn't "toJson" return a JSON rather than a string?
toJson should return a parseable string otherwise it is just toMap with a change of name. @iapicca
I'm saying that this PR doesn't the issue you linked the current problem is that toMap breaks dart:convert as it returns an enum somewhere
I fixed in this PR, but I didn't get any answer in 2 months
your approach seems to be adding a toJson method rather than fixing toMap similar approach as been previously described as not needed by @vbuberen
and quoting@miquelbeltran
I am sorry, I am not sure anymore if we need this. If a user wants to serialize the contents of a DeviceInfo object, they should do it on their own app data model. We shouldn't be maintaining a toJson implementation on our side, even if convenient, it is not our job. Like, I am not even sure why are we providing a toMap method even, I guess that's the legacy from when we got the plugins from Google.
All checks passed on this end... I feel we should leave it to the regulators to decide which approach is right or wrong... I noticed the enum was redundant Because there is already a function and a getter that generates the enum in the first place. So there was never a need for the breaks to happen. But if the maintainers find your approach interesting then I believe they would do the justice.
@miquelbeltran
miquelbeltran
commented
Oct 2, 2022

Copy link


Copy Markdown

Member
@iapicca sorry, I forgot that we had your PR on this, I apologize for not having followed up.
Since the original issue has been closed, I will simply close this PR as well.
As I though originally, I am not in favor of adding extra functionality to the plugins that should be external to the plugin, like JSON support.
@vbuberen
vbuberen
commented
Oct 2, 2022

Copy link


Copy Markdown

Collaborator
similar approach as been previously described as not needed by @vbuberen
Would be good if you could calm down a little bit first, because seems like the miscommunication issue and lack of response from the reviewer of your PR made you feel offended.
Secondly, I wasn't stating anything there, but was genuinely curios to find out what is the reason behind your changes. So would be also good to understand that such question might have a different meaning than what you implied.
@github-actionsgithub-actionsBot
locked as resolved and limited conversation to collaborators
Jan 31, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


No reviews


Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[Request]: make toMap() json compatible


4 participants

@Yczar@miquelbeltran@iapicca@vbuberen

, '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
make toMap() json compatible - #1132
Closed
Yczar wants to merge 5 commits into
fluttercommunity:mainfrom 
Yczar:main
Closed
make toMap() json compatible#1132
Yczar wants to merge 5 commits into
fluttercommunity:mainfrom 
Yczar:main
Conversation
@Yczar
@YczarYczar
commented
Oct 2, 2022•

edited

Loading

Copy link


Copy Markdown

Description
Rendered toMap on the models to be jsonCompatible
Related Issues
Checklist
  •  I read the Contributor Guide and followed the process outlined there for submitting PRs.
  •  My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  •  All existing and new tests are passing.
  •  I updated the version in pubspec.yaml and CHANGELOG.md.
  •  I updated/added relevant documentation (doc comments with ///).
  •  The analyzer (flutter analyze) does not report any problems on my PR.
  •  I read and followed the Flutter Style Guide.
  •  I am willing to follow-up on review comments in a timely manner.
Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?
  •  Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  •  No, this is not a breaking change.
@YczarYczar
mentioned this pull request
Oct 2, 2022

Closed

@miquelbeltran
miquelbeltran
commented
Oct 2, 2022

Copy link


Copy Markdown

Member
An @overrides is missing, check the analyze output.
Would it be possible to add unit tests for the toJson methods?
@Yczar
Yczar
commented
Oct 2, 2022

Copy link


Copy Markdown

Author
An @overrides is missing, check the analyze output.
Would it be possible to add unit tests for the toJson methods?
Fixed these on the latest push @miquelbeltran
@iapicca
iapicca
commented
Oct 2, 2022

Copy link


Copy Markdown

Contributor
@miquelbeltran

shouldn't "toJson" return a json rather than a string?
@Yczar
Yczar
commented
Oct 2, 2022

Copy link


Copy Markdown

Author
@miquelbeltran shouldn't "toJson" return a JSON rather than a string?
toJson should return a parseable string otherwise it is just toMap with a change of name. @iapicca
@iapiccaiapicca
mentioned this pull request
Oct 2, 2022
 Closed

8 tasks
@iapicca
iapicca
commented
Oct 2, 2022

Copy link


Copy Markdown

Contributor
@miquelbeltran shouldn't "toJson" return a JSON rather than a string?
toJson should return a parseable string otherwise it is just toMap with a change of name. @iapicca
I'm saying that this PR doesn't the issue you linked

the current problem is that toMap breaks dart:convert as it returns an enum somewhere
I fixed in this PR,

but I didn't get any answer in 2 months
your approach seems to be adding a toJson method rather than fixing toMap

similar approach as been previously described as not needed by @vbuberen
and quoting@miquelbeltran
I am sorry, I am not sure anymore if we need this. If a user wants to serialize the contents of a DeviceInfo object, they should do it on their own app data model. We shouldn't be maintaining a toJson implementation on our side, even if convenient, it is not our job. Like, I am not even sure why are we providing a toMap method even, I guess that's the legacy from when we got the plugins from Google.
@Yczar
Yczar
commented
Oct 2, 2022

Copy link


Copy Markdown

Author
@miquelbeltran shouldn't "toJson" return a JSON rather than a string?
toJson should return a parseable string otherwise it is just toMap with a change of name. @iapicca
I'm saying that this PR doesn't the issue you linked the current problem is that toMap breaks dart:convert as it returns an enum somewhere
I fixed in this PR, but I didn't get any answer in 2 months
your approach seems to be adding a toJson method rather than fixing toMap similar approach as been previously described as not needed by @vbuberen
and quoting@miquelbeltran
I am sorry, I am not sure anymore if we need this. If a user wants to serialize the contents of a DeviceInfo object, they should do it on their own app data model. We shouldn't be maintaining a toJson implementation on our side, even if convenient, it is not our job. Like, I am not even sure why are we providing a toMap method even, I guess that's the legacy from when we got the plugins from Google.
All checks passed on this end... I feel we should leave it to the regulators to decide which approach is right or wrong... I noticed the enum was redundant Because there is already a function and a getter that generates the enum in the first place. So there was never a need for the breaks to happen. But if the maintainers find your approach interesting then I believe they would do the justice.
@miquelbeltran
miquelbeltran
commented
Oct 2, 2022

Copy link


Copy Markdown

Member
@iapicca sorry, I forgot that we had your PR on this, I apologize for not having followed up.
Since the original issue has been closed, I will simply close this PR as well.
As I though originally, I am not in favor of adding extra functionality to the plugins that should be external to the plugin, like JSON support.
@vbuberen
vbuberen
commented
Oct 2, 2022

Copy link


Copy Markdown

Collaborator
similar approach as been previously described as not needed by @vbuberen
Would be good if you could calm down a little bit first, because seems like the miscommunication issue and lack of response from the reviewer of your PR made you feel offended.
Secondly, I wasn't stating anything there, but was genuinely curios to find out what is the reason behind your changes. So would be also good to understand that such question might have a different meaning than what you implied.
@github-actionsgithub-actionsBot
locked as resolved and limited conversation to collaborators
Jan 31, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


No reviews


Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[Request]: make toMap() json compatible


4 participants

@Yczar@miquelbeltran@iapicca@vbuberen

, '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
make toMap() json compatible - #1132
Closed
Yczar wants to merge 5 commits into
fluttercommunity:mainfrom 
Yczar:main
Closed
make toMap() json compatible#1132
Yczar wants to merge 5 commits into
fluttercommunity:mainfrom 
Yczar:main
Conversation
@Yczar
@YczarYczar
commented
Oct 2, 2022•

edited

Loading

Copy link


Copy Markdown

Description
Rendered toMap on the models to be jsonCompatible
Related Issues
Checklist
  •  I read the Contributor Guide and followed the process outlined there for submitting PRs.
  •  My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  •  All existing and new tests are passing.
  •  I updated the version in pubspec.yaml and CHANGELOG.md.
  •  I updated/added relevant documentation (doc comments with ///).
  •  The analyzer (flutter analyze) does not report any problems on my PR.
  •  I read and followed the Flutter Style Guide.
  •  I am willing to follow-up on review comments in a timely manner.
Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?
  •  Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  •  No, this is not a breaking change.
@YczarYczar
mentioned this pull request
Oct 2, 2022

Closed

@miquelbeltran
miquelbeltran
commented
Oct 2, 2022

Copy link


Copy Markdown

Member
An @overrides is missing, check the analyze output.
Would it be possible to add unit tests for the toJson methods?
@Yczar
Yczar
commented
Oct 2, 2022

Copy link


Copy Markdown

Author
An @overrides is missing, check the analyze output.
Would it be possible to add unit tests for the toJson methods?
Fixed these on the latest push @miquelbeltran
@iapicca
iapicca
commented
Oct 2, 2022

Copy link


Copy Markdown

Contributor
@miquelbeltran

shouldn't "toJson" return a json rather than a string?
@Yczar
Yczar
commented
Oct 2, 2022

Copy link


Copy Markdown

Author
@miquelbeltran shouldn't "toJson" return a JSON rather than a string?
toJson should return a parseable string otherwise it is just toMap with a change of name. @iapicca
@iapiccaiapicca
mentioned this pull request
Oct 2, 2022
 Closed

8 tasks
@iapicca
iapicca
commented
Oct 2, 2022

Copy link


Copy Markdown

Contributor
@miquelbeltran shouldn't "toJson" return a JSON rather than a string?
toJson should return a parseable string otherwise it is just toMap with a change of name. @iapicca
I'm saying that this PR doesn't the issue you linked

the current problem is that toMap breaks dart:convert as it returns an enum somewhere
I fixed in this PR,

but I didn't get any answer in 2 months
your approach seems to be adding a toJson method rather than fixing toMap

similar approach as been previously described as not needed by @vbuberen
and quoting@miquelbeltran
I am sorry, I am not sure anymore if we need this. If a user wants to serialize the contents of a DeviceInfo object, they should do it on their own app data model. We shouldn't be maintaining a toJson implementation on our side, even if convenient, it is not our job. Like, I am not even sure why are we providing a toMap method even, I guess that's the legacy from when we got the plugins from Google.
@Yczar
Yczar
commented
Oct 2, 2022

Copy link


Copy Markdown

Author
@miquelbeltran shouldn't "toJson" return a JSON rather than a string?
toJson should return a parseable string otherwise it is just toMap with a change of name. @iapicca
I'm saying that this PR doesn't the issue you linked the current problem is that toMap breaks dart:convert as it returns an enum somewhere
I fixed in this PR, but I didn't get any answer in 2 months
your approach seems to be adding a toJson method rather than fixing toMap similar approach as been previously described as not needed by @vbuberen
and quoting@miquelbeltran
I am sorry, I am not sure anymore if we need this. If a user wants to serialize the contents of a DeviceInfo object, they should do it on their own app data model. We shouldn't be maintaining a toJson implementation on our side, even if convenient, it is not our job. Like, I am not even sure why are we providing a toMap method even, I guess that's the legacy from when we got the plugins from Google.
All checks passed on this end... I feel we should leave it to the regulators to decide which approach is right or wrong... I noticed the enum was redundant Because there is already a function and a getter that generates the enum in the first place. So there was never a need for the breaks to happen. But if the maintainers find your approach interesting then I believe they would do the justice.
@miquelbeltran
miquelbeltran
commented
Oct 2, 2022

Copy link


Copy Markdown

Member
@iapicca sorry, I forgot that we had your PR on this, I apologize for not having followed up.
Since the original issue has been closed, I will simply close this PR as well.
As I though originally, I am not in favor of adding extra functionality to the plugins that should be external to the plugin, like JSON support.
@vbuberen
vbuberen
commented
Oct 2, 2022

Copy link


Copy Markdown

Collaborator
similar approach as been previously described as not needed by @vbuberen
Would be good if you could calm down a little bit first, because seems like the miscommunication issue and lack of response from the reviewer of your PR made you feel offended.
Secondly, I wasn't stating anything there, but was genuinely curios to find out what is the reason behind your changes. So would be also good to understand that such question might have a different meaning than what you implied.
@github-actionsgithub-actionsBot
locked as resolved and limited conversation to collaborators
Jan 31, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


No reviews


Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[Request]: make toMap() json compatible


4 participants

@Yczar@miquelbeltran@iapicca@vbuberen

, '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
make toMap() json compatible - #1132
Closed
Yczar wants to merge 5 commits into
fluttercommunity:mainfrom 
Yczar:main
Closed
make toMap() json compatible#1132
Yczar wants to merge 5 commits into
fluttercommunity:mainfrom 
Yczar:main
Conversation
@Yczar
@YczarYczar
commented
Oct 2, 2022•

edited

Loading

Copy link


Copy Markdown

Description
Rendered toMap on the models to be jsonCompatible
Related Issues
Checklist
  •  I read the Contributor Guide and followed the process outlined there for submitting PRs.
  •  My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  •  All existing and new tests are passing.
  •  I updated the version in pubspec.yaml and CHANGELOG.md.
  •  I updated/added relevant documentation (doc comments with ///).
  •  The analyzer (flutter analyze) does not report any problems on my PR.
  •  I read and followed the Flutter Style Guide.
  •  I am willing to follow-up on review comments in a timely manner.
Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?
  •  Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  •  No, this is not a breaking change.
@YczarYczar
mentioned this pull request
Oct 2, 2022

Closed

@miquelbeltran
miquelbeltran
commented
Oct 2, 2022

Copy link


Copy Markdown

Member
An @overrides is missing, check the analyze output.
Would it be possible to add unit tests for the toJson methods?
@Yczar
Yczar
commented
Oct 2, 2022

Copy link


Copy Markdown

Author
An @overrides is missing, check the analyze output.
Would it be possible to add unit tests for the toJson methods?
Fixed these on the latest push @miquelbeltran
@iapicca
iapicca
commented
Oct 2, 2022

Copy link


Copy Markdown

Contributor
@miquelbeltran

shouldn't "toJson" return a json rather than a string?
@Yczar
Yczar
commented
Oct 2, 2022

Copy link


Copy Markdown

Author
@miquelbeltran shouldn't "toJson" return a JSON rather than a string?
toJson should return a parseable string otherwise it is just toMap with a change of name. @iapicca
@iapiccaiapicca
mentioned this pull request
Oct 2, 2022
 Closed

8 tasks
@iapicca
iapicca
commented
Oct 2, 2022

Copy link


Copy Markdown

Contributor
@miquelbeltran shouldn't "toJson" return a JSON rather than a string?
toJson should return a parseable string otherwise it is just toMap with a change of name. @iapicca
I'm saying that this PR doesn't the issue you linked

the current problem is that toMap breaks dart:convert as it returns an enum somewhere
I fixed in this PR,

but I didn't get any answer in 2 months
your approach seems to be adding a toJson method rather than fixing toMap

similar approach as been previously described as not needed by @vbuberen
and quoting@miquelbeltran
I am sorry, I am not sure anymore if we need this. If a user wants to serialize the contents of a DeviceInfo object, they should do it on their own app data model. We shouldn't be maintaining a toJson implementation on our side, even if convenient, it is not our job. Like, I am not even sure why are we providing a toMap method even, I guess that's the legacy from when we got the plugins from Google.
@Yczar
Yczar
commented
Oct 2, 2022

Copy link


Copy Markdown

Author
@miquelbeltran shouldn't "toJson" return a JSON rather than a string?
toJson should return a parseable string otherwise it is just toMap with a change of name. @iapicca
I'm saying that this PR doesn't the issue you linked the current problem is that toMap breaks dart:convert as it returns an enum somewhere
I fixed in this PR, but I didn't get any answer in 2 months
your approach seems to be adding a toJson method rather than fixing toMap similar approach as been previously described as not needed by @vbuberen
and quoting@miquelbeltran
I am sorry, I am not sure anymore if we need this. If a user wants to serialize the contents of a DeviceInfo object, they should do it on their own app data model. We shouldn't be maintaining a toJson implementation on our side, even if convenient, it is not our job. Like, I am not even sure why are we providing a toMap method even, I guess that's the legacy from when we got the plugins from Google.
All checks passed on this end... I feel we should leave it to the regulators to decide which approach is right or wrong... I noticed the enum was redundant Because there is already a function and a getter that generates the enum in the first place. So there was never a need for the breaks to happen. But if the maintainers find your approach interesting then I believe they would do the justice.
@miquelbeltran
miquelbeltran
commented
Oct 2, 2022

Copy link


Copy Markdown

Member
@iapicca sorry, I forgot that we had your PR on this, I apologize for not having followed up.
Since the original issue has been closed, I will simply close this PR as well.
As I though originally, I am not in favor of adding extra functionality to the plugins that should be external to the plugin, like JSON support.
@vbuberen
vbuberen
commented
Oct 2, 2022

Copy link


Copy Markdown

Collaborator
similar approach as been previously described as not needed by @vbuberen
Would be good if you could calm down a little bit first, because seems like the miscommunication issue and lack of response from the reviewer of your PR made you feel offended.
Secondly, I wasn't stating anything there, but was genuinely curios to find out what is the reason behind your changes. So would be also good to understand that such question might have a different meaning than what you implied.
@github-actionsgithub-actionsBot
locked as resolved and limited conversation to collaborators
Jan 31, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


No reviews


Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[Request]: make toMap() json compatible


4 participants

@Yczar@miquelbeltran@iapicca@vbuberen