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

Open from filesystem macos #128

Merged
merged 9 commits into from
Feb 22, 2018

Conversation

pklampros
Copy link
Collaborator

Addresses issue #47 for macOS
Also allows application to be understood as able to view/edit .graph files
Also attaches type name to .graph files ("Graph File") and a relevant icon provided by Ugo Santana

@pklampros pklampros requested review from varoudis and blsemo February 13, 2018 15:08
Copy link
Collaborator

@blsemo blsemo left a comment

Choose a reason for hiding this comment

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

Looks good in principle, but see the two comments in the code.

settings.writeSetting(SettingTag::licenseAccepted, true);
}

if (!settings.readSetting(SettingTag::licenseAccepted, false).toBool())
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like a copy & paste error - the license showing code block seems to be duplicated.

}

return QApplication::event(event);
}

int exec() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does this rather hefty function implementation have to be in the header file?

@pklampros pklampros merged commit bd0cb0e into SpaceGroupUCL:master Feb 22, 2018
@pklampros pklampros deleted the open_from_filesystem_macos branch February 22, 2018 02:55
@pklampros pklampros added this to the 0.7.0 milestone Mar 17, 2018
pklampros added a commit that referenced this pull request Sep 2, 2020
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.

2 participants