Skip to content
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

add attachment infos to csv export #20

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

R-Zwi
Copy link

@R-Zwi R-Zwi commented Nov 26, 2024

resolves #17

@R-Zwi
Copy link
Author

R-Zwi commented Nov 26, 2024

This is my first proposal.
But actually the master branch doesn't build in my environment so I developed and tested this on the portable branch and then cherry picked to master.

@@ -31,13 +31,6 @@

#include "sigbak.h"

#define FLAG_EXPORT_ALL 0x1
#define FLAG_FILENAME_ID 0x2
Copy link
Author

Choose a reason for hiding this comment

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

FLAG_FILENAME_ID is also needed in cmd-export-messages.c now. I moved all of those FLAG_* to keep them together.

(att->content_type != NULL) ?
att->content_type : "",
att->size,
sbk_attachment_id_to_string(att));
Copy link
Author

Choose a reason for hiding this comment

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

Ok, if FLAG_FILENAME_ID is given, we get the ID twice (one in the filename and one here).
I think I should change this to not repeat it here when it is already in the filename.
Do you think so, too?

@tbvdm
Copy link
Owner

tbvdm commented Nov 29, 2024

I think you are making too many changes. Please revert the changes to the attachment file logic. I'm not sure that's the right thing to do. Keep only the TAILQ_FOREACH() part in csv_write_message() and use the same text format as the original text_write_attachment_field().

Also, if you can, try keeping to the coding style. See https://man.openbsd.org/style.9 for details. But don't worry about it too much; I can tweak your changes if necessary.

@R-Zwi
Copy link
Author

R-Zwi commented Nov 29, 2024

Thanks for your reply!

I can do that but first I'd like to diskuss if it would be the right thing to do.
The purpose for doing the attachment info different to the text export was to align it with the attachment export. So that one can find the correct file in the exported attachments.

The background for that is that I want to be able to create HTML pages which also show the attachments. That is not possible with the existing attachment info in the text export.
So it comes down to the question if the (new) attachment info in the CSV export should match the existing text export or the upcoming HTML export...

@tbvdm
Copy link
Owner

tbvdm commented Dec 3, 2024

Ah, I see. How is your HTML export going to work? Will it be a new feature in sigbak or a separate program that processes the CSV output?

@R-Zwi
Copy link
Author

R-Zwi commented Dec 3, 2024

Initially I planned to do a Python script to process the CSV output but it turned out to be easy to implement it as a new feature in sigbak, so I'm going this way.
I keep the HTML as simple as possible and leave the formatting to styles.css wherever possible.
So this will be quite basic but sufficient.

Other people (with more knowledge about web designing) might create more fancy output on base of the CSV...

@tbvdm
Copy link
Owner

tbvdm commented Dec 3, 2024

Interesting! But then why do you want to make changes to the CSV format? How is that relevant for an HTML export feature?

@R-Zwi
Copy link
Author

R-Zwi commented Dec 3, 2024

Actually that was required for my first approach with the Python script.
I didn't revert it because it seemd useful to me.
You are right that it isn't required for the HTML export.

@R-Zwi
Copy link
Author

R-Zwi commented Dec 3, 2024

I uploaded my latest changes to my fork so you can have a closer look if you want.

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.

attachment infos in csv export
2 participants