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

closes #53 #54

Merged
merged 6 commits into from
Jan 18, 2021
Merged

closes #53 #54

merged 6 commits into from
Jan 18, 2021

Conversation

infeo
Copy link
Member

@infeo infeo commented Jan 13, 2021

additionally to the refactoring the class AwtFrameworkRevealer is added, which gives library users a default revealer at hand.

additionally to the refactoring the class AwtFrameworkRevealer is added, which gives library users a default revealer at hand.
@infeo infeo requested a review from overheadhunter January 13, 2021 12:29
@infeo infeo linked an issue Jan 13, 2021 that may be closed by this pull request
Copy link
Member

@overheadhunter overheadhunter left a comment

Choose a reason for hiding this comment

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

Right direction, just concerned about exceptions during reveal().

*/
@Deprecated
void revealInFileManager() throws CommandFailedException;
void reveal(Consumer<Path> revealer);
Copy link
Member

Choose a reason for hiding this comment

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

Not sure, if we should define our own FunctionalInterface for revealer to allow throwing checked exceptions..? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

The question is, does it yield a value for the adapter to know that there was a problem during reveal? From my point of view no, because the mount object does not care if it is "revealable" from an external resource (revealer object). It even should only care about itself. If we enforce this caring by a checked exception, it would, from my point of view, only lead to a catch and rethrow.

On the other hand, the caller might know the revealer object and thus can handle possible the fitting, occuring RuntimeExceptions.

Copy link
Member

Choose a reason for hiding this comment

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

The call stack would be:

  1. gui.clickedRevealButton()
  2. mount.reveal(revealer)
  3. revealer.reveal(path)

An exception thrown by 3. is expected to bubble up to 1., so that we can handle the exception where the whole command was invoked. If we want a checked exception, it is required to be thrown during 3. by revealer and not to be caught in 2. by mount.

For unchecked exceptions this is already taken care of, but the question is: Do we want the consumer of the API to be aware of the fact that this might fail? Imo, yes.

Copy link
Member Author

@infeo infeo Jan 14, 2021

Choose a reason for hiding this comment

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

I second, that the API consumer should be informed of possible failure. Unfortunately, due to the inversion of control we have to wrap any exception occuring in revealer.reveal(path) only to unwrap it in the gui layer again, but I guess we cannot have both. Edit: i had a wrong understanding

I'll change the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changes made in 2a4cc57.

The reveal() methods accepts now objects of types Revealer and throws RevealExceptions

Co-authored-by: Sebastian Stenzel <[email protected]>
@infeo infeo self-assigned this Jan 13, 2021
infeo added 2 commits January 14, 2021 12:38
* Add new Revealer and Reveal exception classes
* method accepts Revealer and throws RevealException
Copy link
Member

@overheadhunter overheadhunter left a comment

Choose a reason for hiding this comment

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

Almost good 😉

@infeo infeo added this to the 1.2.7 milestone Jan 18, 2021
@infeo infeo merged commit 989310b into develop Jan 18, 2021
@infeo infeo deleted the feature/#53-refactor-reveal branch January 18, 2021 16:15
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.

Delegate revealing a drive to the API consumer
2 participants