[device_info_plus] make toMap() json compatible - #950

Closed
iapicca wants to merge 12 commits into
fluttercommunity:mainfrom
iapicca:rename_device_info_base_to_map
Closed

[device_info_plus] make toMap() json compatible#950
iapicca wants to merge 12 commits into
fluttercommunity:mainfrom
iapicca:rename_device_info_base_to_map

Conversation

@iapicca

@iapiccaiapicca commented Jul 7, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Description

rename toMap() as toJson() to improve compatibility with dart:convert
see jsonEncode

If toEncodable is omitted, it defaults to a function that returns the result of calling .toJson() on the unencodable object.

updated: makes toMap compatible with json parsing

Related Issues

fix#949

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).
  • [x ] No, this is not a breaking change.

TODO:

after publishing,
bump device_info_plus_platform_interface to ^2.3.1 in device_info_plus's pubspec

@miquelbeltran

miquelbeltran commented Jul 14, 2022

Copy link
Copy Markdown
Member

Thank you! I am positive about this change. The only issue is that this would be a major version bump, so the platform plugin should be increased to 3.0.0, and all its dependencies as well. The change should be well documented in the respective Chanelogs.

Any thoughts? @vbuberen

@vbuberen

vbuberen commented Jul 15, 2022

Copy link
Copy Markdown
Collaborator

Any thoughts? @vbuberen

I am not sure why do we need it.

@iapicca

iapicca commented Jul 15, 2022•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@miquelbeltran
thanks for looking up the PR

@vbuberen
besides the originally stated reason,
during the implementation I also noticed that in some platform toMap is "broken"
more specifically
jsonEncode(info.toMap()) throws because toMap doesn't always return json compatible types (eg: enum)
this PR address this problem as well

@iapiccaiapicca changed the title Rename device info base to maprename BaseDeviceInfo method toMap() as toJson()Jul 15, 2022
@miquelbeltran

miquelbeltran commented Jul 18, 2022

Copy link
Copy Markdown
Member

during the implementation I also noticed that in some platform toMap is "broken" more specifically jsonEncode(info.toMap()) throws because toMap doesn't always return json compatible types (eg: enum) this PR address this problem as well

mmhhh... I am more inclined now to make toJson() a new method rather than modifying the toMap(), that way this is not going to be a breaking change.

What do you think about that @iapicca ?

@iapiccaiapicca changed the title rename BaseDeviceInfo method toMap() as toJson()[device_info_plus] rename BaseDeviceInfo method toMap() as toJson()Jul 18, 2022
@iapicca

iapicca commented Jul 18, 2022•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

mmhhh... I am more inclined now to make toJson() a new method rather than modifying the toMap(), that way this is not going to be a breaking change.

What do you think about that @iapicca ?

@miquelbeltran
no problem, I re-added the toMap() straight from the current upstream main
in most cases I can just toJson() => toMap(), but the web platform interface was messed up (not by me)
and there is a different behavior for both toJson and fromMap
please take a look

@iapicca

This comment was marked as outdated.

Sign in to view
@miquelbeltran

miquelbeltran commented Jul 18, 2022

Copy link
Copy Markdown
Member

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.

@iapicca

iapicca commented Jul 18, 2022

Copy link
Copy Markdown
ContributorAuthor

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.

@miquelbeltran
can I at least fix the web implementation?
It's literally the only case where it cannot be use as jsonEncode(info.toMap())

@miquelbeltran

miquelbeltran commented Jul 18, 2022

Copy link
Copy Markdown
Member

@miquelbeltran can I at least fix the web implementation? It's literally the only case where it cannot be use as jsonEncode(info.toMap())

Yes, I agree with that.

The Readme.md says:

You then use it's toMap method to serialize all known properties to a Map. Your backend service should be prepared to handle new properties, which can be added to this plugin in the future.

If the serialization using toMap doesn't work because it uses enums, then it must be fixed!

@iapicca

iapicca commented Jul 18, 2022

Copy link
Copy Markdown
ContributorAuthor

I did merge properly before, now it's up to date

[...] can I at least fix the web implementation? It's literally the only case where it cannot be use as jsonEncode(info.toMap())

Yes, I agree with that.

The Readme.md says:

You then use it's toMap method to serialize all known properties to a Map. Your backend service should be prepared to handle new properties, which can be added to this plugin in the future.

If the serialization using toMap doesn't work because it uses enums, then it must be fixed!

@miquelbeltran
that's good to hear!
So I removed the toJson and ported the fix for the web implementation
I think this way is more than usable and if someone (like me) really wants toJson
can just do

extensionBaseDeviceInfoToJsonXonBaseDeviceInfo {
Map<String, Object?> Function() get toJson => toMap;
}

@iapiccaiapicca changed the title [device_info_plus] rename BaseDeviceInfo method toMap() as toJson()[device_info_plus] make toMap() json compatibleJul 19, 2022
@iapiccaiapicca mentioned this pull request Jul 19, 2022
Closed
@miquelbeltran

miquelbeltran commented Jul 19, 2022

Copy link
Copy Markdown
Member

Thanks! can you cleanup the PR, and limit the changes to the web plugin implementation?

miquelbeltran
miquelbeltran requested changes Jul 21, 2022
View reviewed changes

@miquelbeltranmiquelbeltran left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All changes not related to the fix should be reverted

Comment threadpackages/device_info_plus/device_info_plus_platform_interface/CHANGELOG.md Outdated
@iapicca

iapicca commented Jul 24, 2022

Copy link
Copy Markdown
ContributorAuthor

Thanks! can you cleanup the PR, and limit the changes to the web plugin implementation?

All changes not related to the fix should be reverted

@miquelbeltran
I think we have to option for the web plugin implementation:

a) remove browserName from both toMap and fromMap

b) keep browserName in both toMap and fromMap ensuring consistency with userAgent data

b is the current implementation and requires the committed changes,
I'm not sure what changes you wish to be limited

@iapiccaiapicca closed this Oct 2, 2022
@iapiccaiapicca mentioned this pull request Oct 2, 2022
Closed
10 tasks
@iapiccaiapicca reopened this Oct 2, 2022
@iapiccaiapicca closed this by deleting the head repository Oct 2, 2022
This was referenced Oct 2, 2022
Closed
Closed
Merged
@nohlinohli mentioned this pull request Jun 24, 2023
Merged
7 tasks
@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

@miquelbeltranmiquelbeltranmiquelbeltran requested changes

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

3 participants

@iapicca@miquelbeltran@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
[device_info_plus] make toMap() json compatible - #950
Closed
iapicca wants to merge 12 commits into
fluttercommunity:mainfrom 
iapicca:rename_device_info_base_to_map
Closed
[device_info_plus] make toMap() json compatible#950
iapicca wants to merge 12 commits into
fluttercommunity:mainfrom 
iapicca:rename_device_info_base_to_map
Conversation
@iapicca
@iapiccaiapicca
commented
Jul 7, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
Description
rename toMap() as toJson() to improve compatibility with dart:convert

see jsonEncode
If toEncodable is omitted, it defaults to a function that returns the result of calling .toJson() on the unencodable object.
updated: makes toMap compatible with json parsing
Related Issues
fix#949
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).
  • [x ] No, this is not a breaking change.
TODO:
after publishing,

bump device_info_plus_platform_interface to ^2.3.1 in device_info_plus's pubspec
@miquelbeltran
miquelbeltran
commented
Jul 14, 2022

Copy link


Copy Markdown

Member
Thank you! I am positive about this change. The only issue is that this would be a major version bump, so the platform plugin should be increased to 3.0.0, and all its dependencies as well. The change should be well documented in the respective Chanelogs.
Any thoughts? @vbuberen
@vbuberen
vbuberen
commented
Jul 15, 2022

Copy link


Copy Markdown

Collaborator
Any thoughts? @vbuberen
I am not sure why do we need it.
@iapicca
iapicca
commented
Jul 15, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@miquelbeltran

thanks for looking up the PR
@vbuberen

besides the originally stated reason,

during the implementation I also noticed that in some platform toMap is "broken"

more specifically
jsonEncode(info.toMap()) throws because toMap doesn't always return json compatible types (eg: enum)

this PR address this problem as well
@iapiccaiapicca
changed the title
Rename device info base to maprename BaseDeviceInfo method toMap() as toJson()Jul 15, 2022
@miquelbeltran
miquelbeltran
commented
Jul 18, 2022

Copy link


Copy Markdown

Member
during the implementation I also noticed that in some platform toMap is "broken" more specifically jsonEncode(info.toMap()) throws because toMap doesn't always return json compatible types (eg: enum) this PR address this problem as well
mmhhh... I am more inclined now to make toJson() a new method rather than modifying the toMap(), that way this is not going to be a breaking change.
What do you think about that @iapicca ?
@iapiccaiapicca
changed the title
rename BaseDeviceInfo method toMap() as toJson()[device_info_plus] rename BaseDeviceInfo method toMap() as toJson()Jul 18, 2022
@iapicca
iapicca
commented
Jul 18, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
mmhhh... I am more inclined now to make toJson() a new method rather than modifying the toMap(), that way this is not going to be a breaking change.
What do you think about that @iapicca ?
@miquelbeltran

no problem, I re-added the toMap() straight from the current upstream main

in most cases I can just toJson() => toMap(), but the web platform interface was messed up (not by me)

and there is a different behavior for both toJson and fromMap

please take a look
@iapicca

This comment was marked as outdated.

Sign in to view
@miquelbeltran
miquelbeltran
commented
Jul 18, 2022

Copy link


Copy Markdown

Member
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.
@iapicca
iapicca
commented
Jul 18, 2022

Copy link


Copy Markdown

ContributorAuthor
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.
@miquelbeltran

can I at least fix the web implementation?

It's literally the only case where it cannot be use as jsonEncode(info.toMap())
@miquelbeltran
miquelbeltran
commented
Jul 18, 2022

Copy link


Copy Markdown

Member
@miquelbeltran can I at least fix the web implementation? It's literally the only case where it cannot be use as jsonEncode(info.toMap())
Yes, I agree with that.
The Readme.md says:
You then use it's toMap method to serialize all known properties to a Map. Your backend service should be prepared to handle new properties, which can be added to this plugin in the future.
If the serialization using toMap doesn't work because it uses enums, then it must be fixed!
@iapicca
iapicca
commented
Jul 18, 2022

Copy link


Copy Markdown

ContributorAuthor
I did merge properly before, now it's up to date
[...] can I at least fix the web implementation? It's literally the only case where it cannot be use as jsonEncode(info.toMap())
Yes, I agree with that.
The Readme.md says:
You then use it's toMap method to serialize all known properties to a Map. Your backend service should be prepared to handle new properties, which can be added to this plugin in the future.
If the serialization using toMap doesn't work because it uses enums, then it must be fixed!
@miquelbeltran

that's good to hear!

So I removed the toJson and ported the fix for the web implementation

I think this way is more than usable and if someone (like me) really wants toJson

can just do
extensionBaseDeviceInfoToJsonXonBaseDeviceInfo {
Map<String, Object?> Function() get toJson => toMap;
}
@iapiccaiapicca
changed the title
[device_info_plus] rename BaseDeviceInfo method toMap() as toJson()[device_info_plus] make toMap() json compatibleJul 19, 2022
@iapiccaiapicca
mentioned this pull request
Jul 19, 2022

Closed

@miquelbeltran
miquelbeltran
commented
Jul 19, 2022

Copy link


Copy Markdown

Member
Thanks! can you cleanup the PR, and limit the changes to the web plugin implementation?
miquelbeltran
miquelbeltran
requested changes
Jul 21, 2022
View reviewed changes
@miquelbeltranmiquelbeltran
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

All changes not related to the fix should be reverted
Comment threadpackages/device_info_plus/device_info_plus_platform_interface/CHANGELOG.md
Outdated

@iapicca
iapicca
commented
Jul 24, 2022

Copy link


Copy Markdown

ContributorAuthor
Thanks! can you cleanup the PR, and limit the changes to the web plugin implementation?
All changes not related to the fix should be reverted
@miquelbeltran

I think we have to option for the web plugin implementation:
a) remove browserName from both toMap and fromMap
b) keep browserName in both toMap and fromMap ensuring consistency with userAgent data
b is the current implementation and requires the committed changes,

I'm not sure what changes you wish to be limited
@iapiccaiapicca
closed this
Oct 2, 2022
@iapiccaiapicca
mentioned this pull request
Oct 2, 2022
 Closed

10 tasks
@iapiccaiapicca
reopened this
Oct 2, 2022
@iapiccaiapicca
closed this by deleting the head repository
Oct 2, 2022

This was referenced Oct 2, 2022

Closed

 Closed

 Merged

@nohlinohli
mentioned this pull request
Jun 24, 2023
 Merged

7 tasks
@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

@miquelbeltranmiquelbeltranmiquelbeltran requested changes

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


3 participants

@iapicca@miquelbeltran@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
[device_info_plus] make toMap() json compatible - #950
Closed
iapicca wants to merge 12 commits into
fluttercommunity:mainfrom 
iapicca:rename_device_info_base_to_map
Closed
[device_info_plus] make toMap() json compatible#950
iapicca wants to merge 12 commits into
fluttercommunity:mainfrom 
iapicca:rename_device_info_base_to_map
Conversation
@iapicca
@iapiccaiapicca
commented
Jul 7, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
Description
rename toMap() as toJson() to improve compatibility with dart:convert

see jsonEncode
If toEncodable is omitted, it defaults to a function that returns the result of calling .toJson() on the unencodable object.
updated: makes toMap compatible with json parsing
Related Issues
fix#949
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).
  • [x ] No, this is not a breaking change.
TODO:
after publishing,

bump device_info_plus_platform_interface to ^2.3.1 in device_info_plus's pubspec
@miquelbeltran
miquelbeltran
commented
Jul 14, 2022

Copy link


Copy Markdown

Member
Thank you! I am positive about this change. The only issue is that this would be a major version bump, so the platform plugin should be increased to 3.0.0, and all its dependencies as well. The change should be well documented in the respective Chanelogs.
Any thoughts? @vbuberen
@vbuberen
vbuberen
commented
Jul 15, 2022

Copy link


Copy Markdown

Collaborator
Any thoughts? @vbuberen
I am not sure why do we need it.
@iapicca
iapicca
commented
Jul 15, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@miquelbeltran

thanks for looking up the PR
@vbuberen

besides the originally stated reason,

during the implementation I also noticed that in some platform toMap is "broken"

more specifically
jsonEncode(info.toMap()) throws because toMap doesn't always return json compatible types (eg: enum)

this PR address this problem as well
@iapiccaiapicca
changed the title
Rename device info base to maprename BaseDeviceInfo method toMap() as toJson()Jul 15, 2022
@miquelbeltran
miquelbeltran
commented
Jul 18, 2022

Copy link


Copy Markdown

Member
during the implementation I also noticed that in some platform toMap is "broken" more specifically jsonEncode(info.toMap()) throws because toMap doesn't always return json compatible types (eg: enum) this PR address this problem as well
mmhhh... I am more inclined now to make toJson() a new method rather than modifying the toMap(), that way this is not going to be a breaking change.
What do you think about that @iapicca ?
@iapiccaiapicca
changed the title
rename BaseDeviceInfo method toMap() as toJson()[device_info_plus] rename BaseDeviceInfo method toMap() as toJson()Jul 18, 2022
@iapicca
iapicca
commented
Jul 18, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
mmhhh... I am more inclined now to make toJson() a new method rather than modifying the toMap(), that way this is not going to be a breaking change.
What do you think about that @iapicca ?
@miquelbeltran

no problem, I re-added the toMap() straight from the current upstream main

in most cases I can just toJson() => toMap(), but the web platform interface was messed up (not by me)

and there is a different behavior for both toJson and fromMap

please take a look
@iapicca

This comment was marked as outdated.

Sign in to view
@miquelbeltran
miquelbeltran
commented
Jul 18, 2022

Copy link


Copy Markdown

Member
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.
@iapicca
iapicca
commented
Jul 18, 2022

Copy link


Copy Markdown

ContributorAuthor
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.
@miquelbeltran

can I at least fix the web implementation?

It's literally the only case where it cannot be use as jsonEncode(info.toMap())
@miquelbeltran
miquelbeltran
commented
Jul 18, 2022

Copy link


Copy Markdown

Member
@miquelbeltran can I at least fix the web implementation? It's literally the only case where it cannot be use as jsonEncode(info.toMap())
Yes, I agree with that.
The Readme.md says:
You then use it's toMap method to serialize all known properties to a Map. Your backend service should be prepared to handle new properties, which can be added to this plugin in the future.
If the serialization using toMap doesn't work because it uses enums, then it must be fixed!
@iapicca
iapicca
commented
Jul 18, 2022

Copy link


Copy Markdown

ContributorAuthor
I did merge properly before, now it's up to date
[...] can I at least fix the web implementation? It's literally the only case where it cannot be use as jsonEncode(info.toMap())
Yes, I agree with that.
The Readme.md says:
You then use it's toMap method to serialize all known properties to a Map. Your backend service should be prepared to handle new properties, which can be added to this plugin in the future.
If the serialization using toMap doesn't work because it uses enums, then it must be fixed!
@miquelbeltran

that's good to hear!

So I removed the toJson and ported the fix for the web implementation

I think this way is more than usable and if someone (like me) really wants toJson

can just do
extensionBaseDeviceInfoToJsonXonBaseDeviceInfo {
Map<String, Object?> Function() get toJson => toMap;
}
@iapiccaiapicca
changed the title
[device_info_plus] rename BaseDeviceInfo method toMap() as toJson()[device_info_plus] make toMap() json compatibleJul 19, 2022
@iapiccaiapicca
mentioned this pull request
Jul 19, 2022

Closed

@miquelbeltran
miquelbeltran
commented
Jul 19, 2022

Copy link


Copy Markdown

Member
Thanks! can you cleanup the PR, and limit the changes to the web plugin implementation?
miquelbeltran
miquelbeltran
requested changes
Jul 21, 2022
View reviewed changes
@miquelbeltranmiquelbeltran
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

All changes not related to the fix should be reverted
Comment threadpackages/device_info_plus/device_info_plus_platform_interface/CHANGELOG.md
Outdated

@iapicca
iapicca
commented
Jul 24, 2022

Copy link


Copy Markdown

ContributorAuthor
Thanks! can you cleanup the PR, and limit the changes to the web plugin implementation?
All changes not related to the fix should be reverted
@miquelbeltran

I think we have to option for the web plugin implementation:
a) remove browserName from both toMap and fromMap
b) keep browserName in both toMap and fromMap ensuring consistency with userAgent data
b is the current implementation and requires the committed changes,

I'm not sure what changes you wish to be limited
@iapiccaiapicca
closed this
Oct 2, 2022
@iapiccaiapicca
mentioned this pull request
Oct 2, 2022
 Closed

10 tasks
@iapiccaiapicca
reopened this
Oct 2, 2022
@iapiccaiapicca
closed this by deleting the head repository
Oct 2, 2022

This was referenced Oct 2, 2022

Closed

 Closed

 Merged

@nohlinohli
mentioned this pull request
Jun 24, 2023
 Merged

7 tasks
@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

@miquelbeltranmiquelbeltranmiquelbeltran requested changes

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


3 participants

@iapicca@miquelbeltran@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
[device_info_plus] make toMap() json compatible - #950
Closed
iapicca wants to merge 12 commits into
fluttercommunity:mainfrom 
iapicca:rename_device_info_base_to_map
Closed
[device_info_plus] make toMap() json compatible#950
iapicca wants to merge 12 commits into
fluttercommunity:mainfrom 
iapicca:rename_device_info_base_to_map
Conversation
@iapicca
@iapiccaiapicca
commented
Jul 7, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
Description
rename toMap() as toJson() to improve compatibility with dart:convert

see jsonEncode
If toEncodable is omitted, it defaults to a function that returns the result of calling .toJson() on the unencodable object.
updated: makes toMap compatible with json parsing
Related Issues
fix#949
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).
  • [x ] No, this is not a breaking change.
TODO:
after publishing,

bump device_info_plus_platform_interface to ^2.3.1 in device_info_plus's pubspec
@miquelbeltran
miquelbeltran
commented
Jul 14, 2022

Copy link


Copy Markdown

Member
Thank you! I am positive about this change. The only issue is that this would be a major version bump, so the platform plugin should be increased to 3.0.0, and all its dependencies as well. The change should be well documented in the respective Chanelogs.
Any thoughts? @vbuberen
@vbuberen
vbuberen
commented
Jul 15, 2022

Copy link


Copy Markdown

Collaborator
Any thoughts? @vbuberen
I am not sure why do we need it.
@iapicca
iapicca
commented
Jul 15, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@miquelbeltran

thanks for looking up the PR
@vbuberen

besides the originally stated reason,

during the implementation I also noticed that in some platform toMap is "broken"

more specifically
jsonEncode(info.toMap()) throws because toMap doesn't always return json compatible types (eg: enum)

this PR address this problem as well
@iapiccaiapicca
changed the title
Rename device info base to maprename BaseDeviceInfo method toMap() as toJson()Jul 15, 2022
@miquelbeltran
miquelbeltran
commented
Jul 18, 2022

Copy link


Copy Markdown

Member
during the implementation I also noticed that in some platform toMap is "broken" more specifically jsonEncode(info.toMap()) throws because toMap doesn't always return json compatible types (eg: enum) this PR address this problem as well
mmhhh... I am more inclined now to make toJson() a new method rather than modifying the toMap(), that way this is not going to be a breaking change.
What do you think about that @iapicca ?
@iapiccaiapicca
changed the title
rename BaseDeviceInfo method toMap() as toJson()[device_info_plus] rename BaseDeviceInfo method toMap() as toJson()Jul 18, 2022
@iapicca
iapicca
commented
Jul 18, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
mmhhh... I am more inclined now to make toJson() a new method rather than modifying the toMap(), that way this is not going to be a breaking change.
What do you think about that @iapicca ?
@miquelbeltran

no problem, I re-added the toMap() straight from the current upstream main

in most cases I can just toJson() => toMap(), but the web platform interface was messed up (not by me)

and there is a different behavior for both toJson and fromMap

please take a look
@iapicca

This comment was marked as outdated.

Sign in to view
@miquelbeltran
miquelbeltran
commented
Jul 18, 2022

Copy link


Copy Markdown

Member
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.
@iapicca
iapicca
commented
Jul 18, 2022

Copy link


Copy Markdown

ContributorAuthor
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.
@miquelbeltran

can I at least fix the web implementation?

It's literally the only case where it cannot be use as jsonEncode(info.toMap())
@miquelbeltran
miquelbeltran
commented
Jul 18, 2022

Copy link


Copy Markdown

Member
@miquelbeltran can I at least fix the web implementation? It's literally the only case where it cannot be use as jsonEncode(info.toMap())
Yes, I agree with that.
The Readme.md says:
You then use it's toMap method to serialize all known properties to a Map. Your backend service should be prepared to handle new properties, which can be added to this plugin in the future.
If the serialization using toMap doesn't work because it uses enums, then it must be fixed!
@iapicca
iapicca
commented
Jul 18, 2022

Copy link


Copy Markdown

ContributorAuthor
I did merge properly before, now it's up to date
[...] can I at least fix the web implementation? It's literally the only case where it cannot be use as jsonEncode(info.toMap())
Yes, I agree with that.
The Readme.md says:
You then use it's toMap method to serialize all known properties to a Map. Your backend service should be prepared to handle new properties, which can be added to this plugin in the future.
If the serialization using toMap doesn't work because it uses enums, then it must be fixed!
@miquelbeltran

that's good to hear!

So I removed the toJson and ported the fix for the web implementation

I think this way is more than usable and if someone (like me) really wants toJson

can just do
extensionBaseDeviceInfoToJsonXonBaseDeviceInfo {
Map<String, Object?> Function() get toJson => toMap;
}
@iapiccaiapicca
changed the title
[device_info_plus] rename BaseDeviceInfo method toMap() as toJson()[device_info_plus] make toMap() json compatibleJul 19, 2022
@iapiccaiapicca
mentioned this pull request
Jul 19, 2022

Closed

@miquelbeltran
miquelbeltran
commented
Jul 19, 2022

Copy link


Copy Markdown

Member
Thanks! can you cleanup the PR, and limit the changes to the web plugin implementation?
miquelbeltran
miquelbeltran
requested changes
Jul 21, 2022
View reviewed changes
@miquelbeltranmiquelbeltran
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

All changes not related to the fix should be reverted
Comment threadpackages/device_info_plus/device_info_plus_platform_interface/CHANGELOG.md
Outdated

@iapicca
iapicca
commented
Jul 24, 2022

Copy link


Copy Markdown

ContributorAuthor
Thanks! can you cleanup the PR, and limit the changes to the web plugin implementation?
All changes not related to the fix should be reverted
@miquelbeltran

I think we have to option for the web plugin implementation:
a) remove browserName from both toMap and fromMap
b) keep browserName in both toMap and fromMap ensuring consistency with userAgent data
b is the current implementation and requires the committed changes,

I'm not sure what changes you wish to be limited
@iapiccaiapicca
closed this
Oct 2, 2022
@iapiccaiapicca
mentioned this pull request
Oct 2, 2022
 Closed

10 tasks
@iapiccaiapicca
reopened this
Oct 2, 2022
@iapiccaiapicca
closed this by deleting the head repository
Oct 2, 2022

This was referenced Oct 2, 2022

Closed

 Closed

 Merged

@nohlinohli
mentioned this pull request
Jun 24, 2023
 Merged

7 tasks
@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

@miquelbeltranmiquelbeltranmiquelbeltran requested changes

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


3 participants

@iapicca@miquelbeltran@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
[device_info_plus] make toMap() json compatible - #950
Closed
iapicca wants to merge 12 commits into
fluttercommunity:mainfrom 
iapicca:rename_device_info_base_to_map
Closed
[device_info_plus] make toMap() json compatible#950
iapicca wants to merge 12 commits into
fluttercommunity:mainfrom 
iapicca:rename_device_info_base_to_map
Conversation
@iapicca
@iapiccaiapicca
commented
Jul 7, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
Description
rename toMap() as toJson() to improve compatibility with dart:convert

see jsonEncode
If toEncodable is omitted, it defaults to a function that returns the result of calling .toJson() on the unencodable object.
updated: makes toMap compatible with json parsing
Related Issues
fix#949
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).
  • [x ] No, this is not a breaking change.
TODO:
after publishing,

bump device_info_plus_platform_interface to ^2.3.1 in device_info_plus's pubspec
@miquelbeltran
miquelbeltran
commented
Jul 14, 2022

Copy link


Copy Markdown

Member
Thank you! I am positive about this change. The only issue is that this would be a major version bump, so the platform plugin should be increased to 3.0.0, and all its dependencies as well. The change should be well documented in the respective Chanelogs.
Any thoughts? @vbuberen
@vbuberen
vbuberen
commented
Jul 15, 2022

Copy link


Copy Markdown

Collaborator
Any thoughts? @vbuberen
I am not sure why do we need it.
@iapicca
iapicca
commented
Jul 15, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@miquelbeltran

thanks for looking up the PR
@vbuberen

besides the originally stated reason,

during the implementation I also noticed that in some platform toMap is "broken"

more specifically
jsonEncode(info.toMap()) throws because toMap doesn't always return json compatible types (eg: enum)

this PR address this problem as well
@iapiccaiapicca
changed the title
Rename device info base to maprename BaseDeviceInfo method toMap() as toJson()Jul 15, 2022
@miquelbeltran
miquelbeltran
commented
Jul 18, 2022

Copy link


Copy Markdown

Member
during the implementation I also noticed that in some platform toMap is "broken" more specifically jsonEncode(info.toMap()) throws because toMap doesn't always return json compatible types (eg: enum) this PR address this problem as well
mmhhh... I am more inclined now to make toJson() a new method rather than modifying the toMap(), that way this is not going to be a breaking change.
What do you think about that @iapicca ?
@iapiccaiapicca
changed the title
rename BaseDeviceInfo method toMap() as toJson()[device_info_plus] rename BaseDeviceInfo method toMap() as toJson()Jul 18, 2022
@iapicca
iapicca
commented
Jul 18, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
mmhhh... I am more inclined now to make toJson() a new method rather than modifying the toMap(), that way this is not going to be a breaking change.
What do you think about that @iapicca ?
@miquelbeltran

no problem, I re-added the toMap() straight from the current upstream main

in most cases I can just toJson() => toMap(), but the web platform interface was messed up (not by me)

and there is a different behavior for both toJson and fromMap

please take a look
@iapicca

This comment was marked as outdated.

Sign in to view
@miquelbeltran
miquelbeltran
commented
Jul 18, 2022

Copy link


Copy Markdown

Member
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.
@iapicca
iapicca
commented
Jul 18, 2022

Copy link


Copy Markdown

ContributorAuthor
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.
@miquelbeltran

can I at least fix the web implementation?

It's literally the only case where it cannot be use as jsonEncode(info.toMap())
@miquelbeltran
miquelbeltran
commented
Jul 18, 2022

Copy link


Copy Markdown

Member
@miquelbeltran can I at least fix the web implementation? It's literally the only case where it cannot be use as jsonEncode(info.toMap())
Yes, I agree with that.
The Readme.md says:
You then use it's toMap method to serialize all known properties to a Map. Your backend service should be prepared to handle new properties, which can be added to this plugin in the future.
If the serialization using toMap doesn't work because it uses enums, then it must be fixed!
@iapicca
iapicca
commented
Jul 18, 2022

Copy link


Copy Markdown

ContributorAuthor
I did merge properly before, now it's up to date
[...] can I at least fix the web implementation? It's literally the only case where it cannot be use as jsonEncode(info.toMap())
Yes, I agree with that.
The Readme.md says:
You then use it's toMap method to serialize all known properties to a Map. Your backend service should be prepared to handle new properties, which can be added to this plugin in the future.
If the serialization using toMap doesn't work because it uses enums, then it must be fixed!
@miquelbeltran

that's good to hear!

So I removed the toJson and ported the fix for the web implementation

I think this way is more than usable and if someone (like me) really wants toJson

can just do
extensionBaseDeviceInfoToJsonXonBaseDeviceInfo {
Map<String, Object?> Function() get toJson => toMap;
}
@iapiccaiapicca
changed the title
[device_info_plus] rename BaseDeviceInfo method toMap() as toJson()[device_info_plus] make toMap() json compatibleJul 19, 2022
@iapiccaiapicca
mentioned this pull request
Jul 19, 2022

Closed

@miquelbeltran
miquelbeltran
commented
Jul 19, 2022

Copy link


Copy Markdown

Member
Thanks! can you cleanup the PR, and limit the changes to the web plugin implementation?
miquelbeltran
miquelbeltran
requested changes
Jul 21, 2022
View reviewed changes
@miquelbeltranmiquelbeltran
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

All changes not related to the fix should be reverted
Comment threadpackages/device_info_plus/device_info_plus_platform_interface/CHANGELOG.md
Outdated

@iapicca
iapicca
commented
Jul 24, 2022

Copy link


Copy Markdown

ContributorAuthor
Thanks! can you cleanup the PR, and limit the changes to the web plugin implementation?
All changes not related to the fix should be reverted
@miquelbeltran

I think we have to option for the web plugin implementation:
a) remove browserName from both toMap and fromMap
b) keep browserName in both toMap and fromMap ensuring consistency with userAgent data
b is the current implementation and requires the committed changes,

I'm not sure what changes you wish to be limited
@iapiccaiapicca
closed this
Oct 2, 2022
@iapiccaiapicca
mentioned this pull request
Oct 2, 2022
 Closed

10 tasks
@iapiccaiapicca
reopened this
Oct 2, 2022
@iapiccaiapicca
closed this by deleting the head repository
Oct 2, 2022

This was referenced Oct 2, 2022

Closed

 Closed

 Merged

@nohlinohli
mentioned this pull request
Jun 24, 2023
 Merged

7 tasks
@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

@miquelbeltranmiquelbeltranmiquelbeltran requested changes

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


3 participants

@iapicca@miquelbeltran@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
[device_info_plus] make toMap() json compatible - #950
Closed
iapicca wants to merge 12 commits into
fluttercommunity:mainfrom 
iapicca:rename_device_info_base_to_map
Closed
[device_info_plus] make toMap() json compatible#950
iapicca wants to merge 12 commits into
fluttercommunity:mainfrom 
iapicca:rename_device_info_base_to_map
Conversation
@iapicca
@iapiccaiapicca
commented
Jul 7, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
Description
rename toMap() as toJson() to improve compatibility with dart:convert

see jsonEncode
If toEncodable is omitted, it defaults to a function that returns the result of calling .toJson() on the unencodable object.
updated: makes toMap compatible with json parsing
Related Issues
fix#949
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).
  • [x ] No, this is not a breaking change.
TODO:
after publishing,

bump device_info_plus_platform_interface to ^2.3.1 in device_info_plus's pubspec
@miquelbeltran
miquelbeltran
commented
Jul 14, 2022

Copy link


Copy Markdown

Member
Thank you! I am positive about this change. The only issue is that this would be a major version bump, so the platform plugin should be increased to 3.0.0, and all its dependencies as well. The change should be well documented in the respective Chanelogs.
Any thoughts? @vbuberen
@vbuberen
vbuberen
commented
Jul 15, 2022

Copy link


Copy Markdown

Collaborator
Any thoughts? @vbuberen
I am not sure why do we need it.
@iapicca
iapicca
commented
Jul 15, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@miquelbeltran

thanks for looking up the PR
@vbuberen

besides the originally stated reason,

during the implementation I also noticed that in some platform toMap is "broken"

more specifically
jsonEncode(info.toMap()) throws because toMap doesn't always return json compatible types (eg: enum)

this PR address this problem as well
@iapiccaiapicca
changed the title
Rename device info base to maprename BaseDeviceInfo method toMap() as toJson()Jul 15, 2022
@miquelbeltran
miquelbeltran
commented
Jul 18, 2022

Copy link


Copy Markdown

Member
during the implementation I also noticed that in some platform toMap is "broken" more specifically jsonEncode(info.toMap()) throws because toMap doesn't always return json compatible types (eg: enum) this PR address this problem as well
mmhhh... I am more inclined now to make toJson() a new method rather than modifying the toMap(), that way this is not going to be a breaking change.
What do you think about that @iapicca ?
@iapiccaiapicca
changed the title
rename BaseDeviceInfo method toMap() as toJson()[device_info_plus] rename BaseDeviceInfo method toMap() as toJson()Jul 18, 2022
@iapicca
iapicca
commented
Jul 18, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
mmhhh... I am more inclined now to make toJson() a new method rather than modifying the toMap(), that way this is not going to be a breaking change.
What do you think about that @iapicca ?
@miquelbeltran

no problem, I re-added the toMap() straight from the current upstream main

in most cases I can just toJson() => toMap(), but the web platform interface was messed up (not by me)

and there is a different behavior for both toJson and fromMap

please take a look
@iapicca

This comment was marked as outdated.

Sign in to view
@miquelbeltran
miquelbeltran
commented
Jul 18, 2022

Copy link


Copy Markdown

Member
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.
@iapicca
iapicca
commented
Jul 18, 2022

Copy link


Copy Markdown

ContributorAuthor
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.
@miquelbeltran

can I at least fix the web implementation?

It's literally the only case where it cannot be use as jsonEncode(info.toMap())
@miquelbeltran
miquelbeltran
commented
Jul 18, 2022

Copy link


Copy Markdown

Member
@miquelbeltran can I at least fix the web implementation? It's literally the only case where it cannot be use as jsonEncode(info.toMap())
Yes, I agree with that.
The Readme.md says:
You then use it's toMap method to serialize all known properties to a Map. Your backend service should be prepared to handle new properties, which can be added to this plugin in the future.
If the serialization using toMap doesn't work because it uses enums, then it must be fixed!
@iapicca
iapicca
commented
Jul 18, 2022

Copy link


Copy Markdown

ContributorAuthor
I did merge properly before, now it's up to date
[...] can I at least fix the web implementation? It's literally the only case where it cannot be use as jsonEncode(info.toMap())
Yes, I agree with that.
The Readme.md says:
You then use it's toMap method to serialize all known properties to a Map. Your backend service should be prepared to handle new properties, which can be added to this plugin in the future.
If the serialization using toMap doesn't work because it uses enums, then it must be fixed!
@miquelbeltran

that's good to hear!

So I removed the toJson and ported the fix for the web implementation

I think this way is more than usable and if someone (like me) really wants toJson

can just do
extensionBaseDeviceInfoToJsonXonBaseDeviceInfo {
Map<String, Object?> Function() get toJson => toMap;
}
@iapiccaiapicca
changed the title
[device_info_plus] rename BaseDeviceInfo method toMap() as toJson()[device_info_plus] make toMap() json compatibleJul 19, 2022
@iapiccaiapicca
mentioned this pull request
Jul 19, 2022

Closed

@miquelbeltran
miquelbeltran
commented
Jul 19, 2022

Copy link


Copy Markdown

Member
Thanks! can you cleanup the PR, and limit the changes to the web plugin implementation?
miquelbeltran
miquelbeltran
requested changes
Jul 21, 2022
View reviewed changes
@miquelbeltranmiquelbeltran
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

All changes not related to the fix should be reverted
Comment threadpackages/device_info_plus/device_info_plus_platform_interface/CHANGELOG.md
Outdated

@iapicca
iapicca
commented
Jul 24, 2022

Copy link


Copy Markdown

ContributorAuthor
Thanks! can you cleanup the PR, and limit the changes to the web plugin implementation?
All changes not related to the fix should be reverted
@miquelbeltran

I think we have to option for the web plugin implementation:
a) remove browserName from both toMap and fromMap
b) keep browserName in both toMap and fromMap ensuring consistency with userAgent data
b is the current implementation and requires the committed changes,

I'm not sure what changes you wish to be limited
@iapiccaiapicca
closed this
Oct 2, 2022
@iapiccaiapicca
mentioned this pull request
Oct 2, 2022
 Closed

10 tasks
@iapiccaiapicca
reopened this
Oct 2, 2022
@iapiccaiapicca
closed this by deleting the head repository
Oct 2, 2022

This was referenced Oct 2, 2022

Closed

 Closed

 Merged

@nohlinohli
mentioned this pull request
Jun 24, 2023
 Merged

7 tasks
@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

@miquelbeltranmiquelbeltranmiquelbeltran requested changes

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


3 participants

@iapicca@miquelbeltran@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
[device_info_plus] make toMap() json compatible - #950
Closed
iapicca wants to merge 12 commits into
fluttercommunity:mainfrom 
iapicca:rename_device_info_base_to_map
Closed
[device_info_plus] make toMap() json compatible#950
iapicca wants to merge 12 commits into
fluttercommunity:mainfrom 
iapicca:rename_device_info_base_to_map
Conversation
@iapicca
@iapiccaiapicca
commented
Jul 7, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
Description
rename toMap() as toJson() to improve compatibility with dart:convert

see jsonEncode
If toEncodable is omitted, it defaults to a function that returns the result of calling .toJson() on the unencodable object.
updated: makes toMap compatible with json parsing
Related Issues
fix#949
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).
  • [x ] No, this is not a breaking change.
TODO:
after publishing,

bump device_info_plus_platform_interface to ^2.3.1 in device_info_plus's pubspec
@miquelbeltran
miquelbeltran
commented
Jul 14, 2022

Copy link


Copy Markdown

Member
Thank you! I am positive about this change. The only issue is that this would be a major version bump, so the platform plugin should be increased to 3.0.0, and all its dependencies as well. The change should be well documented in the respective Chanelogs.
Any thoughts? @vbuberen
@vbuberen
vbuberen
commented
Jul 15, 2022

Copy link


Copy Markdown

Collaborator
Any thoughts? @vbuberen
I am not sure why do we need it.
@iapicca
iapicca
commented
Jul 15, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@miquelbeltran

thanks for looking up the PR
@vbuberen

besides the originally stated reason,

during the implementation I also noticed that in some platform toMap is "broken"

more specifically
jsonEncode(info.toMap()) throws because toMap doesn't always return json compatible types (eg: enum)

this PR address this problem as well
@iapiccaiapicca
changed the title
Rename device info base to maprename BaseDeviceInfo method toMap() as toJson()Jul 15, 2022
@miquelbeltran
miquelbeltran
commented
Jul 18, 2022

Copy link


Copy Markdown

Member
during the implementation I also noticed that in some platform toMap is "broken" more specifically jsonEncode(info.toMap()) throws because toMap doesn't always return json compatible types (eg: enum) this PR address this problem as well
mmhhh... I am more inclined now to make toJson() a new method rather than modifying the toMap(), that way this is not going to be a breaking change.
What do you think about that @iapicca ?
@iapiccaiapicca
changed the title
rename BaseDeviceInfo method toMap() as toJson()[device_info_plus] rename BaseDeviceInfo method toMap() as toJson()Jul 18, 2022
@iapicca
iapicca
commented
Jul 18, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
mmhhh... I am more inclined now to make toJson() a new method rather than modifying the toMap(), that way this is not going to be a breaking change.
What do you think about that @iapicca ?
@miquelbeltran

no problem, I re-added the toMap() straight from the current upstream main

in most cases I can just toJson() => toMap(), but the web platform interface was messed up (not by me)

and there is a different behavior for both toJson and fromMap

please take a look
@iapicca

This comment was marked as outdated.

Sign in to view
@miquelbeltran
miquelbeltran
commented
Jul 18, 2022

Copy link


Copy Markdown

Member
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.
@iapicca
iapicca
commented
Jul 18, 2022

Copy link


Copy Markdown

ContributorAuthor
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.
@miquelbeltran

can I at least fix the web implementation?

It's literally the only case where it cannot be use as jsonEncode(info.toMap())
@miquelbeltran
miquelbeltran
commented
Jul 18, 2022

Copy link


Copy Markdown

Member
@miquelbeltran can I at least fix the web implementation? It's literally the only case where it cannot be use as jsonEncode(info.toMap())
Yes, I agree with that.
The Readme.md says:
You then use it's toMap method to serialize all known properties to a Map. Your backend service should be prepared to handle new properties, which can be added to this plugin in the future.
If the serialization using toMap doesn't work because it uses enums, then it must be fixed!
@iapicca
iapicca
commented
Jul 18, 2022

Copy link


Copy Markdown

ContributorAuthor
I did merge properly before, now it's up to date
[...] can I at least fix the web implementation? It's literally the only case where it cannot be use as jsonEncode(info.toMap())
Yes, I agree with that.
The Readme.md says:
You then use it's toMap method to serialize all known properties to a Map. Your backend service should be prepared to handle new properties, which can be added to this plugin in the future.
If the serialization using toMap doesn't work because it uses enums, then it must be fixed!
@miquelbeltran

that's good to hear!

So I removed the toJson and ported the fix for the web implementation

I think this way is more than usable and if someone (like me) really wants toJson

can just do
extensionBaseDeviceInfoToJsonXonBaseDeviceInfo {
Map<String, Object?> Function() get toJson => toMap;
}
@iapiccaiapicca
changed the title
[device_info_plus] rename BaseDeviceInfo method toMap() as toJson()[device_info_plus] make toMap() json compatibleJul 19, 2022
@iapiccaiapicca
mentioned this pull request
Jul 19, 2022

Closed

@miquelbeltran
miquelbeltran
commented
Jul 19, 2022

Copy link


Copy Markdown

Member
Thanks! can you cleanup the PR, and limit the changes to the web plugin implementation?
miquelbeltran
miquelbeltran
requested changes
Jul 21, 2022
View reviewed changes
@miquelbeltranmiquelbeltran
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

All changes not related to the fix should be reverted
Comment threadpackages/device_info_plus/device_info_plus_platform_interface/CHANGELOG.md
Outdated

@iapicca
iapicca
commented
Jul 24, 2022

Copy link


Copy Markdown

ContributorAuthor
Thanks! can you cleanup the PR, and limit the changes to the web plugin implementation?
All changes not related to the fix should be reverted
@miquelbeltran

I think we have to option for the web plugin implementation:
a) remove browserName from both toMap and fromMap
b) keep browserName in both toMap and fromMap ensuring consistency with userAgent data
b is the current implementation and requires the committed changes,

I'm not sure what changes you wish to be limited
@iapiccaiapicca
closed this
Oct 2, 2022
@iapiccaiapicca
mentioned this pull request
Oct 2, 2022
 Closed

10 tasks
@iapiccaiapicca
reopened this
Oct 2, 2022
@iapiccaiapicca
closed this by deleting the head repository
Oct 2, 2022

This was referenced Oct 2, 2022

Closed

 Closed

 Merged

@nohlinohli
mentioned this pull request
Jun 24, 2023
 Merged

7 tasks
@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

@miquelbeltranmiquelbeltranmiquelbeltran requested changes

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


3 participants

@iapicca@miquelbeltran@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
[device_info_plus] make toMap() json compatible - #950
Closed
iapicca wants to merge 12 commits into
fluttercommunity:mainfrom 
iapicca:rename_device_info_base_to_map
Closed
[device_info_plus] make toMap() json compatible#950
iapicca wants to merge 12 commits into
fluttercommunity:mainfrom 
iapicca:rename_device_info_base_to_map
Conversation
@iapicca
@iapiccaiapicca
commented
Jul 7, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
Description
rename toMap() as toJson() to improve compatibility with dart:convert

see jsonEncode
If toEncodable is omitted, it defaults to a function that returns the result of calling .toJson() on the unencodable object.
updated: makes toMap compatible with json parsing
Related Issues
fix#949
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).
  • [x ] No, this is not a breaking change.
TODO:
after publishing,

bump device_info_plus_platform_interface to ^2.3.1 in device_info_plus's pubspec
@miquelbeltran
miquelbeltran
commented
Jul 14, 2022

Copy link


Copy Markdown

Member
Thank you! I am positive about this change. The only issue is that this would be a major version bump, so the platform plugin should be increased to 3.0.0, and all its dependencies as well. The change should be well documented in the respective Chanelogs.
Any thoughts? @vbuberen
@vbuberen
vbuberen
commented
Jul 15, 2022

Copy link


Copy Markdown

Collaborator
Any thoughts? @vbuberen
I am not sure why do we need it.
@iapicca
iapicca
commented
Jul 15, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@miquelbeltran

thanks for looking up the PR
@vbuberen

besides the originally stated reason,

during the implementation I also noticed that in some platform toMap is "broken"

more specifically
jsonEncode(info.toMap()) throws because toMap doesn't always return json compatible types (eg: enum)

this PR address this problem as well
@iapiccaiapicca
changed the title
Rename device info base to maprename BaseDeviceInfo method toMap() as toJson()Jul 15, 2022
@miquelbeltran
miquelbeltran
commented
Jul 18, 2022

Copy link


Copy Markdown

Member
during the implementation I also noticed that in some platform toMap is "broken" more specifically jsonEncode(info.toMap()) throws because toMap doesn't always return json compatible types (eg: enum) this PR address this problem as well
mmhhh... I am more inclined now to make toJson() a new method rather than modifying the toMap(), that way this is not going to be a breaking change.
What do you think about that @iapicca ?
@iapiccaiapicca
changed the title
rename BaseDeviceInfo method toMap() as toJson()[device_info_plus] rename BaseDeviceInfo method toMap() as toJson()Jul 18, 2022
@iapicca
iapicca
commented
Jul 18, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
mmhhh... I am more inclined now to make toJson() a new method rather than modifying the toMap(), that way this is not going to be a breaking change.
What do you think about that @iapicca ?
@miquelbeltran

no problem, I re-added the toMap() straight from the current upstream main

in most cases I can just toJson() => toMap(), but the web platform interface was messed up (not by me)

and there is a different behavior for both toJson and fromMap

please take a look
@iapicca

This comment was marked as outdated.

Sign in to view
@miquelbeltran
miquelbeltran
commented
Jul 18, 2022

Copy link


Copy Markdown

Member
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.
@iapicca
iapicca
commented
Jul 18, 2022

Copy link


Copy Markdown

ContributorAuthor
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.
@miquelbeltran

can I at least fix the web implementation?

It's literally the only case where it cannot be use as jsonEncode(info.toMap())
@miquelbeltran
miquelbeltran
commented
Jul 18, 2022

Copy link


Copy Markdown

Member
@miquelbeltran can I at least fix the web implementation? It's literally the only case where it cannot be use as jsonEncode(info.toMap())
Yes, I agree with that.
The Readme.md says:
You then use it's toMap method to serialize all known properties to a Map. Your backend service should be prepared to handle new properties, which can be added to this plugin in the future.
If the serialization using toMap doesn't work because it uses enums, then it must be fixed!
@iapicca
iapicca
commented
Jul 18, 2022

Copy link


Copy Markdown

ContributorAuthor
I did merge properly before, now it's up to date
[...] can I at least fix the web implementation? It's literally the only case where it cannot be use as jsonEncode(info.toMap())
Yes, I agree with that.
The Readme.md says:
You then use it's toMap method to serialize all known properties to a Map. Your backend service should be prepared to handle new properties, which can be added to this plugin in the future.
If the serialization using toMap doesn't work because it uses enums, then it must be fixed!
@miquelbeltran

that's good to hear!

So I removed the toJson and ported the fix for the web implementation

I think this way is more than usable and if someone (like me) really wants toJson

can just do
extensionBaseDeviceInfoToJsonXonBaseDeviceInfo {
Map<String, Object?> Function() get toJson => toMap;
}
@iapiccaiapicca
changed the title
[device_info_plus] rename BaseDeviceInfo method toMap() as toJson()[device_info_plus] make toMap() json compatibleJul 19, 2022
@iapiccaiapicca
mentioned this pull request
Jul 19, 2022

Closed

@miquelbeltran
miquelbeltran
commented
Jul 19, 2022

Copy link


Copy Markdown

Member
Thanks! can you cleanup the PR, and limit the changes to the web plugin implementation?
miquelbeltran
miquelbeltran
requested changes
Jul 21, 2022
View reviewed changes
@miquelbeltranmiquelbeltran
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

All changes not related to the fix should be reverted
Comment threadpackages/device_info_plus/device_info_plus_platform_interface/CHANGELOG.md
Outdated

@iapicca
iapicca
commented
Jul 24, 2022

Copy link


Copy Markdown

ContributorAuthor
Thanks! can you cleanup the PR, and limit the changes to the web plugin implementation?
All changes not related to the fix should be reverted
@miquelbeltran

I think we have to option for the web plugin implementation:
a) remove browserName from both toMap and fromMap
b) keep browserName in both toMap and fromMap ensuring consistency with userAgent data
b is the current implementation and requires the committed changes,

I'm not sure what changes you wish to be limited
@iapiccaiapicca
closed this
Oct 2, 2022
@iapiccaiapicca
mentioned this pull request
Oct 2, 2022
 Closed

10 tasks
@iapiccaiapicca
reopened this
Oct 2, 2022
@iapiccaiapicca
closed this by deleting the head repository
Oct 2, 2022

This was referenced Oct 2, 2022

Closed

 Closed

 Merged

@nohlinohli
mentioned this pull request
Jun 24, 2023
 Merged

7 tasks
@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

@miquelbeltranmiquelbeltranmiquelbeltran requested changes

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


3 participants

@iapicca@miquelbeltran@vbuberen