Skip to content
This repository has been archived by the owner on Oct 23, 2019. It is now read-only.

add html formating #92

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

add html formating #92

wants to merge 3 commits into from

Conversation

mr-tron
Copy link

@mr-tron mr-tron commented Oct 21, 2018

Maybe it`s not very pretty code and rendering result, but it works for me.
I copied some styles from telegram desktop exporter.

@Lonami
Copy link
Collaborator

Lonami commented Oct 23, 2018

Looks appealing to me, but any browser will struggle with very large exports (I tried with a 145MB file and stopped Firefox at 4.4GB of memory used). Maybe we would need some sort of pagination? I also found that context is None but that's probably a bug in telegram-export itself, or an old broken database of mine.

Co-Authored-By: mr-tron <denis@subbot.in>
@mr-tron
Copy link
Author

mr-tron commented Oct 23, 2018

Yea, but we can start from small improvments, i think.

@expectocode
Copy link
Owner

I really appreciate you taking the time to work on this @mr-tron, thank you. This has been a feature we've been discussing for a long time.

@mr-tron
Copy link
Author

mr-tron commented Oct 25, 2018

Do you plan to merge this pull request?
For pagination i need support on level begore htmlformatter. maybe baseformatter

@expectocode
Copy link
Owner

Yes, let's start with small improvements as you said. We can build on this.

@expectocode
Copy link
Owner

Some improvements that need to happen before this can be merged:

  • get_forward()'s code style should be made more consistent with the other get_X functions in BaseFormatter. I know it seems small, but consistency makes the code easier to read, and then a reader doesn't have to wonder if get_forward works differently and try to work out the differences.

  • get_human_readable_forwarded seems to me like it belongs at a higher level than BaseFormatter. It only uses public methods of the BaseFormatter, so this shouldn't be difficult. However, when you do move it, please also make its style consistent with other functions (in use of str.format() instead of % formatting, and correct spelling)

  • More style edits in htmlformatter (str.format() again)

I'll use GitHub's tools to annotate specific lines.

Once again, thank you so much for working on this. The reason I ask for these changes is not because I think you've done a bad job, but because I want this code to be a foundation for great things and that starts with fixing these nags.

row = cur.fetchone()
if not row:
return None
row = row[:1] + (datetime.datetime.fromtimestamp(row[1]),) + row[2:]
Copy link
Owner

Choose a reason for hiding this comment

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

See how the other functions do this: they construct the namedtuple first, then use something like _replace(original_date=datetime.datetime.fromtimestamp(original_date)


def get_human_readable_forwarded(self, forward):
if not forward:
return "Unknow forward"
Copy link
Owner

Choose a reason for hiding this comment

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

Typo here.

if forward.from_id:
ent = self.get_user(forward.from_id)
if not ent:
return "Unknown user: %s" % forward.from_id
Copy link
Owner

Choose a reason for hiding this comment

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

Please change these % formats to use "{}".format() style formatting

row = row[:1] + (datetime.datetime.fromtimestamp(row[1]),) + row[2:]
return Forward(*row)

def get_human_readable_forwarded(self, forward):
Copy link
Owner

Choose a reason for hiding this comment

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

As mentioned in my comment, I think this should be moved to a higher level (i.e. htmlformatter)

@@ -4,6 +4,9 @@
"""
from . import BaseFormatter

from .. import utils as export_utils
from .htmltemplates import *
Copy link
Owner

Choose a reason for hiding this comment

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

import * should be avoided if possible. I understand that you want to import a lot of names for templates, but it's totally fine to have a long import line like
from .htmltemplates import column_width, styles, header, basic_message_template, normal_body_template

Alternatively, use a namespaced import so you can do things like templates.styles, templates.basic_message, templates.normal_body

Copy link
Collaborator

Choose a reason for hiding this comment

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

@expectocode you should try the new beta feature from GitHub

```suggestion
from .htmltemplates import column_width, styles, header, basic_message_template, normal_body_template
```

media_path = 'usermedia/%s-%s/%s' %(self.get_display_name(context), context.id, media_path)
body = photo_body_template.format(img_path=media_path)
else:
text = "Unsupported media type: %s. Message_id: %s, media_id: %s" % \
Copy link
Owner

Choose a reason for hiding this comment

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

% formatting

Copy link
Collaborator

Choose a reason for hiding this comment

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

With inconsistent spacing. But yes, use .format().

</div>
"""

forwaded_wrapper_template = """
Copy link
Owner

Choose a reason for hiding this comment

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

Typo here

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
forwaded_wrapper_template = """
forwarded_wrapper_template = """

"""Format the given context as HTML and output to 'file'"""
entity = self.get_entity(context_id)

# file = sys.stdout
Copy link
Owner

Choose a reason for hiding this comment

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

This seems like it should have been deleted

border-bottom: 1px solid #e3e6e8;

}
""" % (column_width, column_width - 20)
Copy link
Owner

Choose a reason for hiding this comment

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

% formatting

@@ -0,0 +1,150 @@
column_width = 480
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps a comment about what this controls?

Copy link
Owner

Choose a reason for hiding this comment

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

Not totally necessary, just a thought. Of course, comments aren't the best solution as they can become outdated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

comments aren't the best solution as they can become outdated.

That's a poor argument. Maybe telegram-export could support --formatter-params column_width=480, or something like that. Having a standard way to define which parameters are valid could also help with error diagnostics, help or just listing options.

@mr-tron
Copy link
Author

mr-tron commented Oct 27, 2018

I am very busy in this days. May be on next week.

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

Successfully merging this pull request may close these issues.

3 participants