Skip to content

Add class that handles ServerValue #335

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Add class that handles ServerValue #335

wants to merge 3 commits into from

Conversation

SUPERCILEX
Copy link
Collaborator

@SUPERCILEX SUPERCILEX commented Oct 2, 2016

  • If this has been discussed in an issue, make sure to mention the issue number here. If not, go file an issue about this to make sure this is a desirable change.
  • If this is a new feature please co-ordinate with someone on FirebaseUI-iOS to make sure that we can implement this on both platforms and maintain feature parity.
    • Anyone?

Example usage:

// After calling ref.getValue(Team.class)

public class Team extends Timestamp {
    public Team() {
        // Example with no config
        getTimestamp();
    }

    // Example getter override
    @PropertyName("timestamp" /* Or any other name as long as you create your own
    getters and setters, but then there is no point in extending Timestamp */)
    public Map<String, String> getCustomTimestamp() {
        if (mShouldUpdateTimestamp) {
            return ServerValue.TIMESTAMP;
        } else {
            return null;
        }
    }

@SUPERCILEX SUPERCILEX changed the title Add class that handles ServerValue Add class that handles ServerValue Oct 2, 2016
@puf
Copy link
Contributor

puf commented Oct 3, 2016

ServerValue.TIMESTAMP also exists on iOS:
https://firebase.google.com/docs/database/ios/offline-capabilities#server-timestamps

On Sun, Oct 2, 2016 at 11:19 AM Alex Saveau [email protected]
wrote:

Example usage:

// After calling ref.getValue(Team.class)
public class Team extends Timestamp {
public Team() {
// Example with no config
getTimestamp();
}

// Example getter override
@PropertyName("timestamp" /* Or any other name as long as you create your own    getters and setters, but then there is no point in extending Timestamp */)
public Map<String, String> getCustomTimestamp() {
    if (mShouldUpdateTimestamp) {
        return ServerValue.TIMESTAMP;
    } else {
        return null;
    }
}

You can view, comment on, or merge this pull request online at:

#335
Commit Summary

File Changes

Patch Links:


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#335, or mute the
thread
https://github.com/notifications/unsubscribe-auth/AA3w32fv7Yx8pu4kf6wLBLmh-TTWkuY-ks5qv_WqgaJpZM4KMEwv
.

Copy link
Collaborator Author

@SUPERCILEX SUPERCILEX left a comment

Choose a reason for hiding this comment

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

This isn't a huge feature so hopefully it will be quick to implement on iOS. I left some thoughts below about null timestamps.

@Exclude
public long getTimestamp() {
return mTimestamp;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should I check to see if mTimestamp is 0 (null) here? If so, I would return the system time. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we'd want to return system time, that's not the same as unset. I prefer how it is now, it's the least opinionated and most likely to solve the original problem (serialization/deserialization of time stamps is annoying)

@samtstern
Copy link
Contributor

@SUPERCILEX I am a little confused by your intended usage. When I first saw this one I imagined it as a class you'd use in an instance variable, something like:

public class MyObject {

   public String name;

   // Easier to use than plain Object
   public Timestamp timestamp;

}

But your PR description implies that the major usage method is extending Timestamp, which I find counter-intuitive. Could you explain a little?

@SUPERCILEX
Copy link
Collaborator Author

@samtstern I was extending just because it was more convenient for me. However, while trying to figure out how to make public Timestamp timestamp work, I ran into some issues. When extending, the data is single level, but when using Timestamp as a variable, I get nested data like so:

timestamp: {
    timestamp: 0000000
},
other data

Is it possible with Firebase to flatten an object into another object? (merge variables from one object into another)

@puf
Copy link
Contributor

puf commented Oct 9, 2016

Nope. The Object graph must exactly match the JSON tree with the default
JSON/POJO codec. Once you do your own mapping, you can of course do
whatever you want.

But for timestamp it'll always be tricky, since the value-type that is
written (JSON: { ".sv": "timestamp" }) is different from the value-type
that you read (number).

On Sun, Oct 9, 2016 at 1:09 AM Alex Saveau [email protected] wrote:

@samtstern https://github.com/samtstern I was extending just because it
was more convenient for me. However, while trying to figure out how to make public
Timestamp timestamp work, I ran into some issues. When extending, the
data is single level, but when using Timestamp as a variable, I get
nested data like so:

timestamp: {
timestamp: 0000000
},
other data

Is it possible with Firebase to flatten an object into another object?
(merge variables from one object into another)


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#335 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA3w3whskr1ZPtmtvEBMnQ3ydRHsvjaHks5qyCK5gaJpZM4KMEwv
.

@SUPERCILEX
Copy link
Collaborator Author

So is it fine to have a nested timestamp like that? I really don't like it, but if you say there is no other way...

@puf
Copy link
Contributor

puf commented Oct 9, 2016

@samstern: can you think of a way to annotate a class to write one type and
read another?

On Sun, Oct 9, 2016 at 9:21 AM Alex Saveau [email protected] wrote:

So is it fine to have a nested timestamp like that? I really don't like
it, but if you say there is no other way...


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#335 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA3w37WmasJt4S7I63a8SAO_r07nwgKYks5qyJYGgaJpZM4KMEwv
.

@samtstern
Copy link
Contributor

@puf sorry for the delay here ... not that I know of. Upon further thought I don't think this feature belongs in FirebaseUI because it will have to be some sort of compromise 😐

@SUPERCILEX
Copy link
Collaborator Author

Closing because of the tradeoffs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants