Skip to content

implement server-time #1108

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Feb 25, 2021
Merged

implement server-time #1108

merged 4 commits into from
Feb 25, 2021

Conversation

ailin-nemui
Copy link
Contributor

No description provided.

@ailin-nemui

This comment has been minimized.

Sign in to view
@ailin-nemui ailin-nemui mentioned this pull request Aug 20, 2019
Closed
@ailin-nemui
Copy link
Contributor Author

to try it, /set show_server_time on

@ailin-nemui ailin-nemui force-pushed the server-time branch 2 times, most recently from 01d2e01 to 006fadf Compare April 6, 2020 09:59
@ailin-nemui ailin-nemui mentioned this pull request Apr 6, 2020
Closed
@ailin-nemui ailin-nemui force-pushed the server-time branch 2 times, most recently from 7ab18e8 to 7e1a96a Compare April 30, 2020 14:52
@ailin-nemui ailin-nemui added the auto-merge This PR is scheduled for merge if no further comments are opened label Apr 30, 2020
@ailin-nemui
Copy link
Contributor Author

@irssi/developers

dequis
dequis reviewed May 2, 2020
View reviewed changes
Copy link
Member

@dequis dequis left a comment

Choose a reason for hiding this comment

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

I like this line meta / current_incoming_meta concept a lot, and the infrastructure you laid to make this possible. Thanks so much for working on this.

src/fe-text/textbuffer-formats.h Outdated
@@ -3,6 +3,11 @@

#include <irssi/src/fe-text/textbuffer.h>

typedef struct _TEXT_BUFFER_META_REC {
gint64 server_time;
GSList *next;
Copy link
Member

Choose a reason for hiding this comment

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

What's this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the meta will consist of pre-parsed meta-data like server_time (to be extended later, but keep in mind that this storage space is required for each line), and arbitrary generic meta-data in a list.

Copy link
Member

Choose a reason for hiding this comment

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

Does that mean that next will be always null for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

@ailin-nemui ailin-nemui removed the auto-merge This PR is scheduled for merge if no further comments are opened label May 2, 2020
@ailin-nemui ailin-nemui force-pushed the server-time branch 4 times, most recently from 60a2c1b to b5f9bb2 Compare May 3, 2020 23:26
@ailin-nemui
Copy link
Contributor Author

I made a hash table implementation

@ailin-nemui
Copy link
Contributor Author

@dequis do you want to review this again? Now we have a tags parser and everything is hashes

@ailin-nemui ailin-nemui force-pushed the server-time branch 2 times, most recently from 8c9f440 to e864467 Compare January 4, 2021 15:19
@ailin-nemui ailin-nemui added auto-merge This PR is scheduled for merge if no further comments are opened needs testing labels Jan 5, 2021
@ailin-nemui
Copy link
Contributor Author

@irssi/developers

@dequis
Copy link
Member

dequis commented Jan 5, 2021

Okay, okay, give me a few days.

@ailin-nemui
Copy link
Contributor Author

@dequis how many days do you want :)?

@dequis
Copy link
Member

dequis commented Jan 11, 2021

Uhm. A few more?

@ailin-nemui
Copy link
Contributor Author

@dequis ping

@dequis
Copy link
Member

dequis commented Feb 24, 2021

Damn time flies huh

dequis
dequis reviewed Feb 24, 2021
View reviewed changes
src/core/refstrings.h Outdated
Comment on lines 10 to 11
void i_refstr_release(char *str);
#define i_refstr_release(str) ((str) == NULL ? NULL : g_ref_string_release(str))
Copy link
Member

Choose a reason for hiding this comment

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

This sounds potentially bad? Having the name defined as both a function and a macro and needing an undef in refstrings.c

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just make all of them functions then?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, good idea, sounds more consistent from api/abi perspective too

@dequis
Copy link
Member

dequis commented Feb 24, 2021

I like this a lot!

I tried really hard to find something to complain about to justify the time it took me to get around to this, and luckily I got something in the end. Phew.

But yeah all good from architectural point of view. I'm a bit rusty on detecting memory bugs visually, we should just fuzz this stuff.

@ailin-nemui
Copy link
Contributor Author

thanks so much for your time and looking through it, much appreciated

@ailin-nemui ailin-nemui removed the auto-merge This PR is scheduled for merge if no further comments are opened label Feb 25, 2021
dequis
dequis approved these changes Feb 25, 2021
View reviewed changes
Copy link
Member

@dequis dequis left a comment

Choose a reason for hiding this comment

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

👍 from the code review perspective, didn't do extra testing myself but we can do that after the merge

@ailin-nemui ailin-nemui merged commit b472570 into irssi:master Feb 25, 2021
@ailin-nemui ailin-nemui deleted the server-time branch February 25, 2021 18:25
@ailin-nemui ailin-nemui mentioned this pull request Feb 26, 2021
Merged
@ailin-nemui ailin-nemui added this to the 1.3.0 milestone Oct 11, 2021
@cwfoo cwfoo mentioned this pull request Oct 23, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dequis dequis dequis approved these changes

Assignees
No one assigned
Labels
ircv3 needs testing
Projects
None yet
Milestone
1.3.0
Development

Successfully merging this pull request may close these issues.
2 participants
@ailin-nemui @dequis