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

Feature/import export/119 #159

Merged
merged 16 commits into from
Apr 25, 2020
Merged

Feature/import export/119 #159

merged 16 commits into from
Apr 25, 2020

Conversation

Moneypulation
Copy link
Member

Description

Import, Export and Reset are now working. Data structure used is a JSON Object of the UserSettings class.
This means that importing/exporting across different iGlance versions might not work because the UserSettings class is not the same

Related Issue

#119

Moneypulation and others added 5 commits April 18, 2020 18:54
…ept network, which has its own issue on github)
app doesnt need to be restarted anymore and cleaned some things up
This reverts commit a367b2d.

revert the latest commit
@Moneypulation Moneypulation linked an issue Apr 22, 2020 that may be closed by this pull request
@D0miH D0miH self-requested a review April 22, 2020 07:33
iGlance/iGlance/iGlance/AppDelegate.swift Outdated Show resolved Hide resolved
iGlance/iGlance/iGlance/AppDelegate.swift Outdated Show resolved Hide resolved
iGlance/iGlance/iGlance/AppDelegate.swift Outdated Show resolved Hide resolved
@@ -177,4 +177,17 @@ class BatteryViewController: MainViewViewController {
// set the first responder to nil in order to loose focus
highBatteryNotificationTextField.window?.makeFirstResponder(highBatteryNotificationTextField.window?.contentView)
}

override func updateGUIComponents() {
Copy link
Member

Choose a reason for hiding this comment

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

to keep the code base consistent it would nice if the overridden functions are at the beginning of the class and marked with

// MARK:   -
// MARK: Function Overrides

updateGUIComponents()
}

public func updateGUIComponents() {
Copy link
Member

Choose a reason for hiding this comment

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

writing public triggers a linter warning because the function has higher access level than the class itself.

Suggested change
public func updateGUIComponents() {
func updateGUIComponents() {

sleep(1)
showMainWindow()
} catch {
let errorAlert = NSAlert()
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use the suggested showErrorModal function here.

@@ -98,8 +101,12 @@ class UserSettings {

private let userSettingsKey: String = "iGlanceUserSettings"

init() {
settings = loadUserSettings()
init(isDefault: Bool) {
Copy link
Member

Choose a reason for hiding this comment

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

IMO it would be cleaner to add a dedicated resetSettings method to this class that overwrites the settings variable. Otherwise you always have to provide the constructor with the isDefault value.

AppDelegate.menuBarItemManager.updateMenuBarItems()

// Restart main window
mainWindow.close()
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have to close and re-instantiate the whole window? Shouldn't this be handled by the updateGUIComponents functions to update the GUI?

We can access the currently displayed subclass of the MainViewViewController with this code:

guard let mainWindowVC = mainWindow.contentViewController as? MainWindowViewController else {
    DDLogError("Could not call the 'contentViewController' of the main window to 'MainWindowViewController'")
    return
}
guard let contentManagerVC = mainWindowVC.contentManagerViewController else {
    DDLogError("'contentManagerViewController' of main window is nil")
    return
}
guard let mainViewVC = contentManagerVC.currentViewController as? MainViewViewController else {
    DDLogError("Could not cast the 'currentViewController' of the 'ContentManagerViewController' to 'MainViewViewController'")
    return
}
mainViewVC.updateGUIComponents()

Maybe we can add some kind of updateGUIofCurrentMainView-function to the private functions above and call it here and in line 270.

override func updateGUIComponents() {
// Call didSet methods of all GUI components
self.versionLabel = { self.versionLabel }()
self.autostartOnBootCheckbox = { self.autostartOnBootCheckbox }()
Copy link
Member

Choose a reason for hiding this comment

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

just calling the didSet method for these settings will set the GUI correctly, but it will not change the behaviour of auto-starting at boot or performing advanced logging.
To change the behaviors accordingly to the settings it is probably best to get the logic in the autoStartCheckboxChanged and the advancedLoggingCheckboxChanged function, put it into private functions which are then called with the appropriate setting values when didSet is executed.

AppDelegate.menuBarItemManager.updateMenuBarItems()

// Restart main window
mainWindow.close()
Copy link
Member

Choose a reason for hiding this comment

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

see comment on line 310

@Moneypulation
Copy link
Member Author

Good ideas for improvement :) They should all be implemented now in the new commit. I created a new file in Utils called Dialog.swift which deals with the NSAlert() stuff and removes the redundant code. I also created Autostart.swift in Utils because after importing/resetting settings, the login item wouldn't get updated (only the setting in the UserSettings class). Same with the logger setting. I implemented a function in Logger.swift that gets called after in import/reset of the settings.

This issue could also be resolved if we implement that stuff in the didSet methods of the IGlanceUserSettings struct. I.e.:

image

But I don't know if it's good style to pack everything into the UserSettings class.

Anyways, resetting and importing now works correctly without the need to restart the App or window (thanks to your code :P) and we now operate on the JSON object of the IGlanceUserSettings struct, not the whole class anymore

khorolets and others added 8 commits April 24, 2020 19:27
* Disk free space widget

* Rename file to conform the style, fix comment to be true

* Add pbxproj file with info about the new class

* added trailing constraint on checkbox

* Small changes and improvements on DiskInfo
* when executing a command the code blocks until the command is done

fixes #160

* fixed PR template
Copy link
Member

@D0miH D0miH left a comment

Choose a reason for hiding this comment

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

top👍🏼

@D0miH D0miH merged commit a6c5ab3 into development Apr 25, 2020
@D0miH D0miH deleted the feature/import-export/119 branch April 25, 2020 17:32
D0miH added a commit that referenced this pull request Apr 26, 2020
* added feature and resolved issue with visibility of menubaritems (except network, which has its own issue on github)

* redesigned this feature

app doesnt need to be restarted anymore and cleaned some things up

* removed unused function

* Revert "removed unused function"

This reverts commit a367b2d.

revert the latest commit

* removed unused functions

* removed 'unavailable_function' linting rule

* Implemented requested changes

* Disk free space widget (#152)

* Disk free space widget

* Rename file to conform the style, fix comment to be true

* Add pbxproj file with info about the new class

* added trailing constraint on checkbox

* Small changes and improvements on DiskInfo

* Bug fix/160 random crashes (#162)

* when executing a command the code blocks until the command is done

fixes #160

* fixed PR template

* changed dev team

* updated cocoa pods

* fixed linting errors

* changed user settings variable back to a constant

* added comment

* added comments

* Feature/import export/119 (#159)

* added feature and resolved issue with visibility of menubaritems (except network, which has its own issue on github)

* redesigned this feature

app doesnt need to be restarted anymore and cleaned some things up

* removed unused function

* Revert "removed unused function"

This reverts commit a367b2d.

revert the latest commit

* removed unused functions

* removed 'unavailable_function' linting rule

* Implemented requested changes

* Disk free space widget (#152)

* Disk free space widget

* Rename file to conform the style, fix comment to be true

* Add pbxproj file with info about the new class

* added trailing constraint on checkbox

* Small changes and improvements on DiskInfo

* Bug fix/160 random crashes (#162)

* when executing a command the code blocks until the command is done

fixes #160

* fixed PR template

* changed dev team

* updated cocoa pods

* fixed linting errors

* changed user settings variable back to a constant

* added comment

* added comments

* Bug fix/161 fix autoupdating (#164)

* SUEnableAutomaticChecks is now enforced to be true

* set the update check interval to 2 hours

* fix freezing dashboard (#165)

fixes #85

* bumped version to 2.0.8

* auto formatting

* added FUNDING.yml file

* updated appcast.xml

Co-authored-by: Bohdan Khorolets <bogdan@khorolets.com>
Co-authored-by: Cemal K <33369079+Moneypulation@users.noreply.github.com>
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.

Import, export and reset the settings of the app
3 participants