Skip to content
This repository was archived by the owner on Dec 16, 2022. It is now read-only.

Loads backlog from filesystem if logging enabled. #391

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Loads backlog from filesystem if logging enabled. #391

wants to merge 7 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 4, 2015

This patch aims to reduce the memory footprint of the backlog. Before it, messages were just accumulated on memory. This adds now the possibility to load messages from the filesystem by seeking and parsing log files. This functionality is only triggered when logging is activated, keeping around 200 messages in memory and loading more if necessary. This modification came out of necessity as I run shout in a very limited VPS with just 128M of memory.

PD. Please consider auto-replacing tabs for spaces in the original code.

@furier
Copy link

furier commented Apr 9, 2015

I created a fork of erming/shout and merged this pull request, for some reason no logs are loaded on shout restart...

@ghost
Copy link
Author

ghost commented Apr 14, 2015

I've removed buffering in memory, it didn't add anything at all. Messages in memory are flushed every 50.

@furier Can you elaborate? I understand you have a set of logs already in disk and shout isn't loading them when you press the button?

@furier
Copy link

furier commented Apr 15, 2015

You are talking about the show more button on the top of the channel? It didn't show up at all for me, the channel just started blank like I had no chat history.

What I did step by step:

  • I merged your pull request into my fork via github
  • Then I ran the following command on the server running shout npm install -g furier/shout which installed my version of shout as shout globally.
  • I then started up shout with the same .shout folder containing all the users and logs from before.

Shout starts up and everything seems to work fine except I cant see that your pull request has changed anything.

@ghost
Copy link
Author

ghost commented Apr 15, 2015

@furier I think you were expecting the logs to be loaded when starting Shout (if they exist for any open channel). That functionality isn't actually implemented. Shout just shows the "show more" button if the channel received more than 100 messages after you joined and it only checks this when a channel is activated (by selecting it on the sidebar). So when the user presses this button some messages were fetched and prepended to the current chat conversation. My patch just makes Shout load those messages to be prepended from logs instead from the messages array in memory. You can just tell the difference by checking memory consumption with a reasonable number of channels open.

@furier
Copy link

furier commented Apr 15, 2015

Okay then I misunderstood the intent of the pull request, sorry @aynik. :)

@williamboman
Copy link
Contributor

I noticed there are some coding style issues here. Spaces and tabs mixed as indentation character (tabs should be used afaik), missing whitespaces in function signatures (function(){} -> function () {}), mixing quotation mark character.

These are not enforced in any way, so it's easy to miss. I'll open an issue for this.

@furier
Copy link

furier commented Sep 28, 2015

tabs should be converted to spaces always, use spaces always over tabs...

man. 28. sep. 2015 kl. 08.08 skrev William Boman [email protected]:

I noticed there are some coding style issues here. Spaces and tabs mixed
as indentation character (tabs should be used afaik), missing whitespaces
in function signatures (function(){} -> function () {}), mixing quotation
mark character.

These are not enforced in any way, so it's easy to miss. I'll open an
issue for this.


Reply to this email directly or view it on GitHub
#391 (comment).

@williamboman
Copy link
Contributor

@furier That's a personal preference. All/most code in this repo is indented with tabs so far.

@furier
Copy link

furier commented Sep 28, 2015

I have never ever worked at a project where they use tabs as standard indentation characters, usually its 4 spaces instead of a tab. Most IDEs also use 4 spaces instead of a tab, but be my guest... :)

@williamboman
Copy link
Contributor

I have never ever worked at a project where they use tabs as standard indentation characters, usually its 4 spaces instead of a tab.

http://stackoverflow.com/research/developer-survey-2015#tech-tabsspaces

If the entire project is already indented with tabs, then tabs it is - unless maintainers want to convert them to spaces and completely ruin git blame, git log -- file, etc. It's not a matter of personal preference this far in 😄. FWIW, I also prefer 4 spaces indentation

edit: obligatory

@astorije
Copy link
Collaborator

Alright alright, let's focus on this PR, gentlepeople :-)

However, my comments before burying that nice bikeshed:

I noticed there are some coding style issues here. Spaces and tabs mixed as indentation character (tabs should be used afaik), missing whitespaces in function signatures (function(){} -> function () {}), mixing quotation mark character.

Yes indeed. @aynik, I know there is no style checker yet, but if you could comply with what's already there, that would be great for consistency and would make the usage of a style checker doable :-) Once you change what @williamboman mentions, I'll point at other potential culprits.

tabs should be converted to spaces always, use spaces always over tabs...

That's a fine argument of why one should always use tabs that you have here :P

I have never ever worked at a project where they use tabs as standard indentation characters, usually its 4 spaces instead of a tab.

I have. It wasn't the best time of my life, but you know, you get used to it after the first 5 seconds. Also, what I like more than 4-space indentation is 2-space indentation, because 4-space indentation really hurts when you are trying to stay within 80-char lines, and you should (unless you like to hate yourself when coding from a plane, a bus, ... anywhere you use your 13" screen 😄). But what I like even more than 2-space indentation : ✨ consistency

Most IDEs also use 4 spaces instead of a tab, but be my guest... :)

Well, the last 3 code editors I have been using for the last few years, and that are now very-widely used, favored 4-space indentations (and my current one doesn't care that much, it overwrites the default with the currently opened file's preference). So "Some IDEs", yes, "Most IDEs", no.

WARNING: Please do not feed any troll nor answer my comments about {1-9,tab}-based indentation or other personal preferences in this chain of comment. Open an issue if you like, but let's keep this PR about the PR. Thank you.

@ghost
Copy link
Author

ghost commented Sep 28, 2015

Sure, I didn't mean to hurt anybody's feelings. 😁
Not adding function definition spacing though, compact style seems to be the norm.

+1 Applying standard style to the whole project.

@qrohlf
Copy link

qrohlf commented Dec 4, 2015

What's the status of this PR? I'd love to see this feature in master, as I am also running shout in a very low-memory environment.

@spencerthayer
Copy link

I'm subscribing to this because it's an important feature I would like to see soon.

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

Successfully merging this pull request may close these issues.

6 participants