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

Settings update #7

Merged
merged 31 commits into from
Apr 15, 2024
Merged

Settings update #7

merged 31 commits into from
Apr 15, 2024

Conversation

cd-FileX
Copy link
Contributor

  • Added new settings and changed old for new functions
  • Changed order and grouped settings
  • Changed config.md to new version & html for correct display
  • Added settings help button for additional descriptions of config.md

Partly connected to future pull requests

cd-FileX and others added 21 commits April 15, 2024 16:18
- Added new settings and changed old for new functions
- Changed order and grouped settings
- Changed config.md to new version & html for correct display
- Added settings help button for additional descriptions
- Delete previous log file content upon opening
  - Saves time when closing anki and storage
- Added button to settings to view log file
__init__.py Outdated
Comment on lines 4 to 8
# Roadmap:
# - Improve upper part of search window (more options(?), fast switch for online, collapsible(?))
# - Improve online request (to-card-converter, importer)
# - Update search UI to immersion kit like style and add option to merge local and online cards into one search

Copy link
Member

Choose a reason for hiding this comment

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

I think it's better move these lines to Issues instead of committing them in code.

If anything, this should be in a different PR.

common.py Outdated
Comment on lines 53 to 55
# clear before writing to avoid large logfiles and their consequences
open(DEBUG_LOG_FILE_PATH, 'w').close()
self._logfile = open(DEBUG_LOG_FILE_PATH, "a")
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if it's desirable to delete the contents of the log file when Anki opens. The user might need the logs from previous runs. If the user doesn't want to keep a debug log, it's better to have it disabled. (set enable_debug_log to false in Options.)

Copy link
Member

Choose a reason for hiding this comment

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

open(DEBUG_LOG_FILE_PATH, 'w').close() and open(DEBUG_LOG_FILE_PATH, "a") can be simplified to open(DEBUG_LOG_FILE_PATH, 'w')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case we should maybe build in something like splitted logfiles, a button to clear the log file or making the view opened in "Open Logfile" editable.
It is a bit of a pain if you are using anki for a long time and it needs a few moments to cool down and close the log.

Copy link
Member

@tatsumoto-ren tatsumoto-ren Apr 16, 2024

Choose a reason for hiding this comment

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

In that case we should maybe build in something like splitted logfiles, a button to clear the log file or making the view opened in "Open Logfile" editable. It is a bit of a pain if you are using anki for a long time and it needs a few moments to cool down and close the log.

The existing logging code was written by Russel Simmons a long time ago. Later I refactored it a bit, but the logic is mostly still the same. Today we should just do:

from aqt.addons import AddonManager
logger = AddonManager.get_logger(__name__)
logger.info("my message")

If we use Anki's logging, it will automatically clear old logs (older than 10 days by default).

See #8

common.py Outdated
Comment on lines 62 to 67
def read(self):
if not self._logfile:
return ""
with open(self._logfile.name, 'r') as lf:
return lf.read()

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why it's done like this, but I think this would be easier

    def read(self):
        with open(DEBUG_LOG_FILE_PATH, 'r', encoding='utf-8') as lf:
            return lf.read()

config.json Outdated
Comment on lines 2 to 24
"max_displayed_notes": 500,
"exported_tag": "cropro_exported",
"notes_per_page": 100,
"hidden_fields": [
"furigana",
"image",
"audio"
],
"skip_duplicates": true,
"copy_tags": true,
"show_note_preview": true,
"search_online": false,
"show_extended_filters": true,
"show_help_buttons": true,

"import_source_info": false,
"fetch_anki_card_media": false,

"allow_empty_search": false,
"preview_on_right_side": true,
"copy_card_data": false,
"call_add_cards_hook": false,
"exported_tag": "cropro_exported",

"timeout_seconds": 60,
"enable_debug_log": false,
"search_the_web": false,
"timeout_seconds": 60
"call_add_cards_hook": false
Copy link
Member

Choose a reason for hiding this comment

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

If you rename existing config keys, the users will lose their settings. Additionally, it makes the diff very hard to read and understand what's changed.

config.py Outdated

@property
def enable_debug_log(self) -> bool:
return self["enable_debug_log"]
Copy link
Member

Choose a reason for hiding this comment

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

The positions of the existing settings are changed. This makes the diff more difficult to read.

Comment on lines 36 to 38
self.button_box = QDialogButtonBox(
(BUT_HELP | BUT_OK | BUT_CANCEL) if config.show_help_buttons else (BUT_OK | BUT_CANCEL)
)
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need to be able to disable the help button? If it's always visible, it won't harm anybody.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, implemented without thinking about it again.
Was from the previous version where you got the a bit weirdly placed help button in the search window.
Thanks for the hint

Comment on lines 41 to 53
def _setup_ui(self) -> None:
self.setMinimumWidth(300)
self.setWindowTitle(f"{ADDON_NAME} Settings")
self.setLayout(self._make_layout())

Copy link
Member

Choose a reason for hiding this comment

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

This method was moved. It makes the diff harder to read.

config.json Outdated
"show_extended_filters": true,
"show_help_buttons": true,
"import_source_info": false,
"fetch_anki_card_media": false
Copy link
Member

Choose a reason for hiding this comment

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

This config key fetch_anki_card_media appears unused. I assume it needs to be implemented in a different PR.

config.json Outdated
"timeout_seconds": 60,
"show_extended_filters": true,
"show_help_buttons": true,
"import_source_info": false,
Copy link
Member

Choose a reason for hiding this comment

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

This config key import_source_info appears unused. I assume it needs to be implemented in a different PR.

config.json Outdated
@@ -14,5 +14,9 @@
"call_add_cards_hook": false,
"enable_debug_log": false,
"search_the_web": false,
"timeout_seconds": 60
"timeout_seconds": 60,
"show_extended_filters": true,
Copy link
Member

Choose a reason for hiding this comment

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

This config key show_extended_filters appears unused. I assume it needs to be implemented in a different PR.

@tatsumoto-ren tatsumoto-ren merged commit bdc50f2 into Ajatt-Tools:main Apr 15, 2024
@tatsumoto-ren
Copy link
Member

I have removed the config options that were not implemented. They need to be added in separate pull requests.

@tatsumoto-ren
Copy link
Member

Also removed the roadmap and config deletion (see the review).

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.

None yet

2 participants