Skip to content

Updates for External Files, Menus, and custom fields#2

Open
givemeahigh5 wants to merge 8 commits into
Jasig:masterfrom
givemeahigh5:master
Open

Updates for External Files, Menus, and custom fields#2
givemeahigh5 wants to merge 8 commits into
Jasig:masterfrom
givemeahigh5:master

Conversation

@givemeahigh5
Copy link
Copy Markdown

@givemeahigh5 givemeahigh5 commented Oct 22, 2016

Patches to better serve multi-OS delivery and app customization:

  • Updated bundling plugins
  • Updated file path resolution in Configuration and MainForm, including Windows/Mac distinction and utilizing ApplicationPath() instead of getApplcationPath()
  • Modified processing of Saved Taxa Sets to match other external files/directories
  • Updated HelpPopup and alert dialogs to allow custom text on welcome dialog, plus custom icon and application name to be set by external file
  • Removed functionality to install a new database

Copy link
Copy Markdown
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Thanks for your pull request!

Code generally looks good.
I left a few questions and comments.

Some general orders of business for contributing:

  1. Could you sign the Apereo ICLA? https://www.apereo.org/licensing/agreements
  2. Could you report this as a feature request on this portlet's issue tracker? https://issues.jasig.org/browse/IQ
  3. Could you include a summary of the change in the pull request?
  4. optional Screenshots of changes that affect the appearance of the portlet are helpful
  5. optional This looks like a significant change, the uPortal dev group may be interested https://groups.google.com/a/apereo.org/forum/#!forum/uportal-dev

JOptionPane.showMessageDialog(this.jtable.getParent(),
"Invalid Number");
Utilities.MessageDialog(this.jtable.getParent(), "Invalid Number");
//JOptionPane.showMessageDialog(this.jtable.getParent(), "Invalid Number");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

could the commented out JOptionPane thoughout the code be removed?

Comment thread pom.xml Outdated
<version>40</version>
</parent>

<!-- updated ztesler, 04-2016 -->
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

git automatically tracks which user most recently edited a given line of code.

Comment thread pom.xml Outdated
<groupId>org.apereo.application</groupId>
<artifactId>ImageQuiz</artifactId>
<version>1.0-SNAPSHOT</version>
<version>2.0</version>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

jumping to version 2 before version 1 is tagged?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@bjagg should the version be 1.0.0 to fit the major.minor.patch format of semver?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Following semantic versioning, this is likely 1.0.1 (if only patches) or 1.1.0 if there are new features. Since it looks compatible with previous versions, it really does not look like a 2.0 release.

Thanks Christian for the review!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since there's been no 1.0.0 release, I think the <version> shouldn't increment at all. Start observing Semantic Versioning after 1.0.0. Before then it's just a wild west snapshot.

@apetro
Copy link
Copy Markdown
Member

apetro commented Feb 4, 2017

I verify a name matching @givemeahigh5 's name appears among the names of people who have signed ICLAs.

@apetro
Copy link
Copy Markdown
Member

apetro commented Feb 4, 2017

I grabbed the branch behind this pull request, built and ran locally. The resulting onejar works no worse for me than does status quo Jasig/master. ~~~In both cases all I get is the initial dialog to choose an existing or create a new user, and then after that, no further dialogs or windows, just nothing.~~~

EDIT: @BKKirchoff kindly lent me some quiz content for testing. I successfully ran the onejar from this branch against that data. Passes smoke test. I can't say I exercised all the specific changes in this changeset, but spot checking the application overall it worked great.

@givemeahigh5
Copy link
Copy Markdown
Author

Additional assets, including a database and graphics, are needed locally to run the app -- without them nothing shows up after the login screen. There are multiple versions (i.e. plants, herpetology) managed separately and not included here. @apetro it sounds like maybe you lacked these. I can run both Jasig/master and my local branch without incident, though the versioning and comment issues above still need to be addressed.

Copy link
Copy Markdown
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks @givemeahigh5!

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.

4 participants