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

Reading view mode: Basic functionality #44

Merged
merged 42 commits into from
Mar 11, 2019

Conversation

rlrh
Copy link

@rlrh rlrh commented Mar 5, 2019

Resolves #45

Resolves part of #27

How to use:

view reader for reading view
view browser for browser view

How it works:

When reading view is selected, BrowserPanel loads the original web page first, then inputs the resulting HTML into Crux for processing, and finally loads the cleaned-up HTML.

@rlrh rlrh marked this pull request as ready for review March 10, 2019 06:02
@rlrh rlrh changed the title Basic reading view mode Reading view mode: Basic functionality Mar 10, 2019
@coveralls
Copy link

coveralls commented Mar 10, 2019

Pull Request Test Coverage Report for Build 220

  • 115 of 132 (87.12%) changed or added relevant lines in 9 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.6%) to 93.352%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/main/java/seedu/address/logic/LogicManager.java 1 3 33.33%
src/main/java/seedu/address/model/ModelManager.java 14 16 87.5%
src/main/java/seedu/address/commons/util/XmlUtil.java 6 10 60.0%
src/main/java/seedu/address/ui/BrowserPanel.java 71 80 88.75%
Totals Coverage Status
Change from base Build 217: -0.6%
Covered Lines: 1657
Relevant Lines: 1775

💛 - Coveralls

@rlrh
Copy link
Author

rlrh commented Mar 11, 2019

Test coverage dropped by 0.7% as it is very difficult to test for certain cases.

return stateCheck;
} else {
return stateCheck && exception.get().getMessage().equals(other.exception.get().getMessage());
}

Choose a reason for hiding this comment

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

It's possible one exception is null but the other isn't.

Copy link
Author

Choose a reason for hiding this comment

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

Oh yeah, I'll fix this.

return output;
}

private static Document convertStringToDocument(String xmlStr) throws Exception {

Choose a reason for hiding this comment

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

throws Exception do we have a more specific type here?

Copy link
Author

Choose a reason for hiding this comment

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

There's a couple: javax.xml.parsers.ParserConfigurationException, org.xml.sax.SAXException, java io.IOException

private ViewMode viewMode;
private Consumer<CommandResult> onSuccess;

Choose a reason for hiding this comment

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

We'll merge this for now but note this is why we usually don't want to do any processing on the UI side :p

Copy link
Author

Choose a reason for hiding this comment

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

The most recent commit doesn't have the CommandResult consumer any more.

Copy link

@thomastanck thomastanck left a comment

Choose a reason for hiding this comment

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

lgtm

@rlrh rlrh merged commit d32a649 into CS2103-AY1819S2-W10-1:master Mar 11, 2019
@thomastanck thomastanck deleted the basic-view-mode branch March 11, 2019 13:02
@rlrh rlrh added this to the v1.2 milestone Mar 12, 2019
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

3 participants