-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: profile for commands #1
Conversation
Warning Rate Limit Exceeded@Lisandra-dev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 33 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe project has evolved to refine user customization and improve functionality. By removing certain properties and introducing Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Review Status
Actionable comments generated: 10
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
src/i18n/locales/en.json
is excluded by:!**/*.json
src/i18n/locales/fr.json
is excluded by:!**/*.json
Files selected for processing (7)
- src/interface.ts (2 hunks)
- src/main.ts (9 hunks)
- src/modal.ts (1 hunks)
- src/settings.ts (6 hunks)
- src/utils/NodesEdit.ts (1 hunks)
- src/utils/conversion.ts (6 hunks)
- src/utils/selection.ts (4 hunks)
Additional comments: 16
src/interface.ts (4)
- 78-79: The addition of the
profiles
property to theEnhancedCopySettings
interface aligns with the PR objectives to support customizable profiles. Ensure that the typeGlobalSettings[]
is correctly used throughout the codebase whereEnhancedCopySettings
is utilized.- 99-100: The
overrideNativeCopy
property in theediting
section ofDEFAULT_SETTINGS
is correctly set tofalse
by default, which is consistent with the expected behavior of not overriding the native copy functionality unless explicitly enabled by the user.- 108-111: The addition of
spaceReadingSize
andwikiToMarkdown
properties in thereading
section ofDEFAULT_SETTINGS
with their default values (-1
andfalse
, respectively) is appropriate. These settings provide users with more customization options for reading mode.- 118-132: The
GlobalSettings
interface now includes optional properties that enhance customization (name
,copyAsHTML
,spaceReadingSize
,tabToSpace
,tabSpaceSize
,wikiToMarkdown
,applyingTo
). This change supports the PR's objective of allowing users to define different profiles with specific settings. Ensure that these properties are correctly handled in the code whereGlobalSettings
is used.src/utils/selection.ts (3)
- 4-4: Renaming the import from
EnhancedCopySettings
toGlobalSettings
correctly reflects the updated settings structure. This change should be consistent across all files where settings are utilized.- 27-34: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [15-31]
The update in
getSelectionAsHTML
to usesettings.copyAsHTML
instead of the previously usedsettings.exportAsHTML
aligns with the new settings structure. This change correctly implements the logic to determine whether the selection should be returned as HTML based on the profile settings.
- 117-117: Passing
settings.reading
togetSelectionAsHTML
in thecanvasSelectionText
function instead of the entiresettings
object is a logical adjustment that ensures only relevant settings are used for processing canvas selections. This change is consistent with the new profile-based settings system.src/utils/NodesEdit.ts (1)
- 56-56: The update in
replaceAllDivCalloutToBlockquote
to handledivCallout.parentElement
being null by providing a fallback empty string forcalloutType
is a necessary improvement. This change prevents potential null reference errors and ensures the function's robustness.src/modal.ts (1)
- 193-235: The
NameProfile
class correctly extendsModal
and includes properties and methods for handling profile naming and submission. The implementation ofonOpen
for setting up the modal content andonClose
for cleaning up ensures proper modal behavior. This addition supports the PR's objective of enhancing user interaction and customization through UI elements.src/utils/conversion.ts (4)
- 113-116: The logic in
fixFootnoteContents
for handling footnotes based on settings (remove
orformat
) is correctly implemented. The use of regex to adjust footnote formatting or removal aligns with the expected behavior. Ensure that this logic is consistently applied across all markdown processing functions.- 191-193: The
convertTabToSpace
function correctly checkssettings.tabToSpace
before performing the conversion, using thetabSpaceSize
setting for the number of spaces. This implementation supports customizable tab-to-space conversion based on user profiles.- 199-208: The
convertSpaceSize
function's handling ofsettings.spaceReadingSize
to adjust space sizes in markdown content is logically structured. However, ensure that the division by 4 (space.length / 4
) is always applicable and consider edge cases where the original content might not use 4 spaces for tabs.- 252-252: The
convertMarkdown
function's chaining of conversion functions to process markdown based on settings is well-structured. This approach ensures that all necessary conversions are applied in a logical order, aligning with the new settings system.src/main.ts (3)
- 1-22: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [19-29]
The
enhancedCopy
method's introduction of an optionalprofile
parameter and its handling within the method align with the PR's objectives to support customizable profiles. Ensure that the method correctly applies the provided profile settings or falls back to default settings when no profile is specified.
- 131-151: The loop to add commands for each profile in the settings is correctly implemented, allowing users to invoke specific profile settings through commands. Ensure that the command IDs are unique and that the profile names are sanitized to avoid potential issues with command registration.
- 229-235: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [232-244]
The logic to register event listeners for overriding native copy functionality based on settings (
reading.overrideNativeCopy
orediting.overrideNativeCopy
) is correctly implemented. However, ensure that the cleanup of event listeners and the restoration of default behavior are properly handled when the plugin is unloaded or when the active leaf changes.
createReadingSettings(settings: GlobalSettings, profile?: boolean) { | ||
this.settingsPage.createEl("h1", { text: i18next.t("reading.desc") }); | ||
new Setting(this.settingsPage) | ||
.setName(i18next.t("copyAsHTML")) | ||
.addToggle((toggle) => { | ||
toggle | ||
.setValue(settings.copyAsHTML ?? false) | ||
.onChange(async (value) => { | ||
settings.copyAsHTML = value; | ||
await this.plugin.saveSettings(); | ||
this.renderSettingsPage(settings.name ?? "reading"); | ||
}); | ||
}); | ||
if (!settings.copyAsHTML) { | ||
this.settingsPage.createEl("h2", { text: i18next.t("links") }); | ||
this.links(settings); | ||
this.footnotes(settings); | ||
|
||
this.settingsPage.createEl("h2", { text: i18next.t("unconventionalMarkdown.title") }); | ||
this.settingsPage.createEl("i", { text: i18next.t("unconventionalMarkdown.desc") }); | ||
this.highlight(settings); | ||
} | ||
|
||
this.calloutTitle(settings); | ||
|
||
if (!settings.copyAsHTML) { | ||
this.settingsPage.createEl("h2", { text: i18next.t("other") }); | ||
this.hardBreak(settings); | ||
new Setting(this.settingsPage) | ||
.setName(i18next.t("spaceSize.title")) | ||
.setDesc(i18next.t("spaceSize.desc")) | ||
.addText((text) => { | ||
text | ||
.setPlaceholder("-1") | ||
.setValue(String(settings.spaceReadingSize ?? -1)) | ||
.onChange(async (value) => { | ||
settings.spaceReadingSize = Number(value); | ||
await this.plugin.saveSettings(); | ||
}); | ||
}); | ||
} | ||
if (!profile) this.overrideSetting(settings); | ||
this.regexReplacementButton(settings); | ||
} |
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.
Consider refactoring createReadingSettings
to reduce duplication and improve maintainability. The method contains repeated blocks for creating UI elements and handling settings changes. Extracting these into reusable functions could simplify the code and make future modifications easier.
createEditSettings(settings: GlobalSettings, profile?: boolean) { | ||
this.settingsPage.createEl("h1", { text: i18next.t("edit.desc") }); | ||
new Setting(this.settingsPage) | ||
.setName(i18next.t("wikiToMarkdown.title")) | ||
.setDesc(i18next.t("wikiToMarkdown.desc")) | ||
.addToggle((toggle) => { | ||
toggle | ||
.setValue(settings.wikiToMarkdown ?? false) | ||
.onChange(async (value) => { | ||
settings.wikiToMarkdown = value; | ||
await this.plugin.saveSettings(); | ||
this.renderSettingsPage(settings.name ?? "edit"); | ||
}); | ||
}); | ||
|
||
new Setting(this.settingsPage) | ||
.setName(i18next.t("tabToSpace")) | ||
.addToggle((toggle) => { | ||
toggle | ||
.setValue(settings.tabToSpace ?? false) | ||
.onChange(async (value) => { | ||
settings.tabToSpace = value; | ||
await this.plugin.saveSettings(); | ||
this.renderSettingsPage(settings.name ?? "edit"); | ||
}); | ||
}); | ||
|
||
if (settings.tabToSpace) { | ||
new Setting(this.settingsPage) | ||
.setName(i18next.t("tabSpaceSize")) | ||
.addText((text) => { | ||
text | ||
.setValue(settings.tabSpaceSize ? settings.tabSpaceSize.toString() : "4") | ||
.onChange(async (value) => { | ||
settings.tabSpaceSize = parseInt(value); | ||
text.inputEl.toggleClass("error", isNaN(settings.tabSpaceSize) || settings.tabSpaceSize < 0); | ||
if (isNaN(settings.tabSpaceSize) || settings.tabSpaceSize < 0) settings.tabSpaceSize = 4; | ||
await this.plugin.saveSettings(); | ||
}); | ||
}); | ||
} | ||
this.settingsPage.createEl("h2", { text: i18next.t("links") }); | ||
if (settings.wikiToMarkdown) { | ||
this.links(settings); | ||
} | ||
this.footnotes(settings); | ||
this.settingsPage.createEl("h2", { text: i18next.t("unconventionalMarkdown.title") }); | ||
this.settingsPage.createEl("i", { text: i18next.t("unconventionalMarkdown.desc") }); | ||
this.calloutTitle(settings); | ||
this.highlight(settings); | ||
this.settingsPage.createEl("h2", { text: i18next.t("other") }); | ||
this.hardBreak(settings); | ||
if (!profile) this.overrideSetting(settings); | ||
this.regexReplacementButton(settings); | ||
|
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.
Similar to createReadingSettings
, createEditSettings
also exhibits code duplication in UI element creation and settings handling. Apply the same refactoring suggestions to improve code maintainability and reduce duplication.
this.TABS.push(this.EDIT); | ||
this.TABS.push(this.READING); | ||
} | ||
// remove duplicate | ||
|
||
for (const profile of this.settings.profiles) { | ||
//remove all previous profile | ||
this.TABS = this.TABS.filter((tab) => tab.id !== profile.name); | ||
this.TABS.push({ | ||
name: profile.name ?? "truc", | ||
id: profile.name ?? "profile", | ||
icon: "code" | ||
}); | ||
} | ||
this.TABS = [...new Set(this.TABS)]; | ||
|
||
for (const tabInfo of this.TABS) { | ||
const tabEl = tabBar.createEl("div", {cls: "settings-tab"}); | ||
const tabIcon = tabEl.createEl("div", {cls: "settings-tab-icon"}); | ||
const tabEl = tabBar.createEl("div", { cls: "settings-tab" }); | ||
const tabIcon = tabEl.createEl("div", { cls: "settings-tab-icon" }); | ||
setIcon(tabIcon, tabInfo.icon); | ||
if (tabInfo.id === "global") | ||
tabEl.addClasses(["settings-tab-active"]); | ||
tabEl.createEl("div", {cls: "tabName", text: tabInfo.name}); | ||
tabEl.createEl("div", { cls: "tabName", text: tabInfo.name }); | ||
tabEl.addEventListener("click", () => { | ||
// @ts-ignore | ||
for (const tabEl of tabBar.children) |
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.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [150-172]
The logic for modifying this.TABS
based on this.settings.applyingTo
and adding profiles to this.TABS
could lead to unexpected behavior due to direct manipulation of the array. Consider using a more declarative approach to define this.TABS
based on the current state, which could enhance readability and reduce potential bugs.
src/settings.ts
Outdated
if (this.settings.profiles.find((profile) => profile.name === tab)) { | ||
this.renderProfile(tab); |
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.
The check for a profile's existence in renderSettingsPage
before calling renderProfile
is redundant since renderProfile
already performs a null check. Removing this redundant check could streamline the code.
src/settings.ts
Outdated
renderProfile(tab: string) { | ||
const profile = this.settings.profiles.find((profile) => profile.name === tab); | ||
if (!profile) return; | ||
this.settingsPage.empty(); | ||
new Setting(this.settingsPage) | ||
.addButton((button) => { | ||
button | ||
.setButtonText(i18next.t("profile.delete")) | ||
.onClick(async () => { | ||
const index = this.settings.profiles.findIndex((profile) => profile.name === tab); | ||
this.settings.profiles.splice(index, 1); | ||
//remove from tabs | ||
this.TABS = this.TABS.filter((tab) => tab.id !== profile.name); | ||
this.plugin.saveSettings(); | ||
this.display(); | ||
this.renderGlobal(); | ||
}) | ||
.buttonEl.classList.add("full-width"); | ||
}) | ||
.infoEl.classList.add("hide-info"); | ||
profile.applyingTo = profile.applyingTo ?? ApplyingToView.all; | ||
this.settingsPage.createEl("h1", { text: profile.name }); | ||
new Setting(this.settingsPage) | ||
.setName(i18next.t("view.title")) | ||
.setDesc(i18next.t("view.desc")) | ||
.addDropdown((dropdown) => { | ||
dropdown | ||
.addOption("all", i18next.t("view.all")) | ||
.addOption("reading", i18next.t("view.reading")) | ||
.addOption("edit", i18next.t("view.edit")) | ||
.setValue(profile.applyingTo ?? ApplyingToView.all) | ||
.onChange(async (value) => { | ||
profile.applyingTo = value as ApplyingToView; | ||
await this.plugin.saveSettings(); | ||
this.renderSettingsPage(tab); | ||
}); | ||
}); | ||
if (profile.applyingTo === ApplyingToView.all) { | ||
this.createReadingSettings(profile, true); | ||
this.createEditSettings(profile, true); | ||
} else if (profile.applyingTo === ApplyingToView.reading) { | ||
this.createReadingSettings(profile, true); | ||
} | ||
else if (profile.applyingTo === ApplyingToView.edit) { | ||
this.createEditSettings(profile, true); | ||
} | ||
|
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.
In renderProfile
, there's a pattern of creating UI elements and handling settings changes that's similar to other parts of the file. This repetition suggests an opportunity for further refactoring. Consider creating a utility class or functions for common UI patterns to reduce code duplication and improve maintainability.
new Setting(this.settingsPage) | ||
.addButton((button) => { | ||
button | ||
.setButtonText(i18next.t("profile.add.title")) | ||
.setTooltip(i18next.t("profile.add.desc")) | ||
.onClick(async () => { | ||
new NameProfile(this.app, (result) => { | ||
this.settings.profiles.push({ | ||
name: result, | ||
...this.settings.editing | ||
}); | ||
this.plugin.saveSettings(); | ||
this.display(); | ||
}).open(); | ||
|
||
}); | ||
}); |
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.
The method for adding a profile in renderGlobal
uses an inline callback for the onClick
event, which could be extracted to a separate method for better readability and potential reuse. Additionally, consider validating the profile name before adding it to ensure it meets any necessary criteria (e.g., non-empty, unique).
highlight(settings: GlobalSettings) { | ||
return new Setting(this.settingsPage) | ||
.setName(i18next.t("highlight.title")) | ||
.setDesc(i18next.t("highlight.desc")) | ||
.addToggle((toggle) => { | ||
toggle | ||
.setValue(this.settings.tabToSpace) | ||
.setValue(settings.highlight) | ||
.onChange(async (value) => { | ||
this.settings.tabToSpace = value; | ||
settings.highlight = value; |
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.
The highlight
method is a good candidate for the suggested refactoring to reduce UI element creation duplication. Extracting common patterns into reusable functions or a utility class could simplify this and similar methods.
overrideSetting(settings: GlobalSettings) { | ||
return new Setting(this.settingsPage) | ||
.setName(i18next.t("highlight.title")) | ||
.setDesc(i18next.t("highlight.desc")) | ||
.setName(i18next.t("overrideCopy.title")) | ||
.setDesc(i18next.t("overrideCopy.desc")) | ||
.addToggle((toggle) => { | ||
toggle | ||
.setValue(settings.highlight) | ||
.setValue(settings.overrideNativeCopy) | ||
.onChange(async (value) => { | ||
settings.highlight = value; | ||
settings.overrideNativeCopy = value; |
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.
The overrideSetting
method, like highlight
, could benefit from the same refactoring suggestions to improve code maintainability and reduce duplication.
@@ -140,7 +139,7 @@ function removeMarkdownFootNotes(markdown: string, overrides: GlobalSettings): s | |||
* @returns {string} Markdown with highlight removed if needed | |||
*/ | |||
function removeHighlightMark(markdown: string, settings: GlobalSettings): string { | |||
if (settings.highlight ) { | |||
if (settings.highlight) { |
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.
The removeHighlightMark
function's conditional check for settings.highlight
and subsequent removal of highlight marks is correctly implemented. However, the logic seems inverted; it should remove the highlight marks if settings.highlight
is false
, not true
. Verify the intended behavior and adjust accordingly.
- if (settings.highlight) {
+ if (!settings.highlight) {
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
if (settings.highlight) { | |
if (!settings.highlight) { |
src/main.ts
Outdated
const exportAsHTML = profile ? profile?.copyAsHTML ?? false : this.settings.reading.copyAsHTML; | ||
const applyingTo = profile?.applyingTo ?? this.settings.applyingTo; | ||
if (selectedText && selectedText.trim().length > 0) { | ||
if (!this.settings.exportAsHTML && | ||
(this.settings.applyingTo === ApplyingToView.all || this.settings.applyingTo === viewIn) | ||
if (!exportAsHTML && | ||
(applyingTo === ApplyingToView.all || applyingTo === viewIn) | ||
) { | ||
console.log(profile); | ||
selectedText = viewIn === ApplyingToView.edit | ||
? | ||
convertEditMarkdown(selectedText, this.settings.editing, this) : | ||
convertMarkdown(selectedText, this.settings.reading, this); | ||
convertEditMarkdown(selectedText, profile ?? this.settings.editing, this) : | ||
convertMarkdown(selectedText, profile ?? this.settings.reading, 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.
The logic within enhancedCopy
to determine whether to export as HTML or convert markdown based on the selected profile or default settings is correctly implemented. However, the console log on line 52 seems to be for debugging purposes and should be removed or guarded by a development mode check.
- console.log(profile);
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
const exportAsHTML = profile ? profile?.copyAsHTML ?? false : this.settings.reading.copyAsHTML; | |
const applyingTo = profile?.applyingTo ?? this.settings.applyingTo; | |
if (selectedText && selectedText.trim().length > 0) { | |
if (!this.settings.exportAsHTML && | |
(this.settings.applyingTo === ApplyingToView.all || this.settings.applyingTo === viewIn) | |
if (!exportAsHTML && | |
(applyingTo === ApplyingToView.all || applyingTo === viewIn) | |
) { | |
console.log(profile); | |
selectedText = viewIn === ApplyingToView.edit | |
? | |
convertEditMarkdown(selectedText, this.settings.editing, this) : | |
convertMarkdown(selectedText, this.settings.reading, this); | |
convertEditMarkdown(selectedText, profile ?? this.settings.editing, this) : | |
convertMarkdown(selectedText, profile ?? this.settings.reading, this); | |
const exportAsHTML = profile ? profile?.copyAsHTML ?? false : this.settings.reading.copyAsHTML; | |
const applyingTo = profile?.applyingTo ?? this.settings.applyingTo; | |
if (selectedText && selectedText.trim().length > 0) { | |
if (!exportAsHTML && | |
(applyingTo === ApplyingToView.all || applyingTo === viewIn) | |
) { | |
selectedText = viewIn === ApplyingToView.edit | |
? | |
convertEditMarkdown(selectedText, profile ?? this.settings.editing, this) : | |
convertMarkdown(selectedText, profile ?? this.settings.reading, this); |
Summary by CodeRabbit
divCallout.parentElement
being null caused errors in text conversion.