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

Add a button / hook for reverting to default settings #25

Merged
merged 2 commits into from
Apr 6, 2021

Conversation

peddie
Copy link
Contributor

@peddie peddie commented Mar 31, 2021

No description provided.

src/SetHo.hs Outdated Show resolved Hide resolved
src/SetHo.hs Outdated
@@ -194,6 +204,9 @@ runSetter mconfig rootName initialValue userPollForNewMessage sendRequest userCo

_ <- on buttonDiff Gtk.buttonActivated printDiff

_ <- on buttonRevertToDefaults Gtk.buttonActivated
((getLatestStaged >>= revertToDefaults) *> takeLatestUpstream)
Copy link
Owner

Choose a reason for hiding this comment

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

I haven't looked at this code in a while, but I don't immediately understand this. Can you add a short comment on the use case? Specifically:

  • why do you need the latest staged DTree in order to revert to defaults?
  • why do you call takeLatestUpstream afterwards? in a GCS use-case the userRevertToDefaults won't have an affect by the time takeLatestUpstream is called, is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason to call takeLatestUpstream was so that reverting to defaults in the upstream system would also revert the staged values. It appears you are correct that this doesn't work as I'd hoped. I've deleted it; the user will just have to revert the staged values with a separate button press.

src/SetHo.hs Outdated
@@ -39,8 +39,8 @@ defaultSetHoConfig =
}

-- | fire up the the GUI
runSetter :: Maybe SetHoConfig -> String -> DTree -> IO (Maybe (Int, DTree)) -> (Int -> IO ()) -> (Int -> DTree -> IO ()) -> IO ()
runSetter mconfig rootName initialValue userPollForNewMessage sendRequest userCommit = do
runSetter :: Maybe SetHoConfig -> String -> DTree -> IO (Maybe (Int, DTree)) -> (Int -> IO ()) -> (Int -> DTree -> IO ()) -> (Int -> DTree -> IO ()) -> IO ()
Copy link
Owner

Choose a reason for hiding this comment

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

This breaks the binary in the examples directory, can you please update it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm unable to figure out with Cabal how to actually build this example. I nixified all the dependencies and can build the library, but:

peddie@jaidee:Plot-ho-matic(master☠)$ nix-shell --pure shell.nix --run "ghcid --command 'cabal v2-build set-example'"
Loading cabal v2-build set-example ...
Resolving dependencies...
Build profile: -w ghc-8.10.4 -O1
In order, the following will be built (use -v for more details):
 - Plot-ho-matic-0.12.2.3 (exe:set-example) (configuration changed)
Configuring executable 'set-example' for Plot-ho-matic-0.12.2.3..
Warning: The package has an extraneous version range for a dependency on an
internal library: Plot-ho-matic >=0 && ==0.12.2.3, Plot-ho-matic >=0 &&
==0.12.2.3. This version range includes the current package but isn't needed
as the current package's library will always be used.
Preprocessing executable 'set-example' for Plot-ho-matic-0.12.2.3..
Building executable 'set-example' for Plot-ho-matic-0.12.2.3..
Command "cabal v2-build set-example" exited unexpectedly with error message: as the current package's library will always be used.

The incomplete error message, combined with the problematic version bounds on Plot-ho-matic which appear nowhere in the actual .cabal file, make me think this might be a cabal bug (shocker). Removing Plot-ho-matic from the build-depends: field of the executable does not solve the problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haskell/cabal#5119 looks like it is a bug in cabal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyway, I think I fixed the example, but I can't check that it compiles thanks to cabal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked this with stack as recommended after rebasing on master, and it works.

@ghorn ghorn merged commit 7bd9e88 into ghorn:master Apr 6, 2021
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