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

Created a new Preferences window, and added PromptSaveOnExit option. #1106

Closed
wants to merge 0 commits into from
Closed

Conversation

EliteMasterEric
Copy link

For tutorials installed to a read-only location, it may be disorienting for the user to be prompted to save. For this reason, I created a preferences dialog window, with a boolean option for whether to Prompt Save on Exit, defaulting to true. These preferences are saved to any config .rviz files that are created.

@wjwwood
Copy link
Member

wjwwood commented Aug 4, 2017

@mastereric sorry for the long delay in reviewing this. Looks like there are some merge conflicts now. One of us may look at resolving the conflicts soon, but if you have time that would be appreciated.

@EliteMasterEric
Copy link
Author

EliteMasterEric commented Aug 7, 2017

@wjwwood The commit has been rebased to resolve conflicts with the base branch.

One thing to note about the preferences window is that I added it with the intent of creating infrastructure to allow myself or other users to create additional options in the future. It only has one option because I only had one feature I wanted to add.

@EliteMasterEric
Copy link
Author

@wjwwood It has been 17 days since this pull request was last reviewed. There are currently no conflicts, and if you see no problem with the changes I would like them merged.

@wjwwood
Copy link
Member

wjwwood commented Aug 24, 2017

@mastereric one of us needs to compile it and try it out. We'll get to as soon as possible. Thanks for your patience.

@EliteMasterEric
Copy link
Author

Hey @wjwwood,
I noticed that it has been more than 6 months since this pull request was made, but no feedback has been provided on it. Please compile and test the code and get back to me with feedback soon.

@dhood
Copy link
Contributor

dhood commented Mar 5, 2018

Hey @mastereric, you're right that this has taken a long time to get reviewed; we will try to look at it this week/next. One thing I'm able to say is now that, unfortunately, since the PR will break ABI, it is not likely we will be able to release this into existing distros, but instead will have to hold for the Melodic release even if it gets approved. I'll get back to you with a more thorough review once I've tested it out.

@dhood
Copy link
Contributor

dhood commented Apr 6, 2018

rebasing your branch closed the PR accidentally. I opened #1216 in its place and will comment on that PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants