-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Desktop, Mobile: Resolves #10664: Allow user to generate deletion logs #11083
Conversation
if (!Logger.globalLogger) { | ||
throw new Error('Could not find global logger'); | ||
} | ||
|
||
const fileTarget = Logger.globalLogger.targets().find(t => t.type === TargetType.File); | ||
|
||
if (!fileTarget || !fileTarget.path) { | ||
throw new Error('Log file target not found'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the log file was recently rotated, you won't get the information. Maybe instead just directly get log.txt and the latest log-*.txt from the profile directory?
|
||
await shim.fsDriver().writeFile(deletionLogPath, deletionLog, 'utf8'); | ||
|
||
await void bridge().openItem(Setting.value('profileDir')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead please open the file you've just created. Seems a bit random to open the profile directory as the user won't know why we're doing this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also no await void
please
name: 'exportDeletionLogs', | ||
label: () => _('Export deletion logs'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be singular deletion log. Also the filename should be singular
I updated the PR getting all the log files, including the rotations one. I also included a function to order by the timestamp in the log filename (while every time I tested it was already in the right order, I thought it would be better to include this logic there in case something changes) Since I'm building the path with pure strings (aka: |
const logsOrderedByTimestamp = (logFiles: Stat[]) => { | ||
return logFiles.sort((a, b) => { | ||
const aTimestamp = a.path.match(/\d+/); | ||
const bTimestamp = b.path.match(/\d+/); | ||
|
||
if (!aTimestamp) return 1; | ||
if (!bTimestamp) return -1; | ||
|
||
return parseInt(aTimestamp[0], 10) - parseInt(bTimestamp[0], 10); | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be the same to sort alphabetically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, thanks for pointing it out, I'm removing this in favour of sort()
if (!Logger.globalLogger) { | ||
throw new Error('Could not find global logger'); | ||
} | ||
|
||
const fileTarget = Logger.globalLogger.targets().find(t => t.type === TargetType.File); | ||
|
||
if (!fileTarget || !fileTarget.path) { | ||
throw new Error('Log file target not found'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get why this is still here
for (const file of orderedLogs) { | ||
|
||
const deletionLines = await getDeletionLines(file.path); | ||
|
||
allDeletionLines += deletionLines; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've suggested taking log.txt and the latest log-*.txt, nothing more. We don't need to look at the last 2 years of logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests?
if (!Logger.globalLogger) { | ||
throw new Error('Could not find global logger'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, why do you need this check for? You don't use the logger anywhere in that code below.
allDeletionLines += deletionLines; | ||
} | ||
|
||
const deletionLogPath = `${Setting.value('profileDir')}/deletion_log.txt`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please append a date to the file name - eg. deletion_log_20241123.txt
Resolves #10664
Summary
We are adding this new feature on Desktop and Mobile to help users share logs about deletion when they might want some help figuring out what is happening.
To do that, on mobile I created a new Log button on the Tools submenu that filters every DeleteAction event.
On Desktop, I created a Desktop-specific command that reads the log of the active profile and filters every line with the
DeleteAction
event, creating a newdeletion_logs.txt
file on the current profile directory.Testing
I only did manual tests, like:
1 - Create a notebook and a new note
2 - Send the note to the trash
3 - Empty the trash
4 - Create a new note, and send it to the trash
5 - Access the new log screen on mobile or
Export deletion logs
on the desktop6 - Only the note that isn't in trash anymore (because it was permanently deleted) should be logged