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

Added memory of directory and filename to export & save dialogs #5873

Merged
merged 34 commits into from
Mar 1, 2021

Conversation

Patowhiz
Copy link
Contributor

@Patowhiz Patowhiz commented Jun 29, 2020

Fixes #5606
Also Fixes #5589. (minor design changes)
Also Fixes #5602
Also Fixes #5590

This PR affects 4 dialogs dlgExportDataset, dlgExportGraphAsImage , dlgExportRObjects , dlgExportRWorkspace , dlgSaveAs

@africanmathsinitiative/developers this is ready for review

dlgExportDataset
dlgExportGraphAsImage
dlgExportRObjects
dlgExportRWorkspace
dlgSaveAs
@lloyddewit
Copy link
Contributor

@Ivanluv Please could you review this PR? thanks

@Patowhiz
Copy link
Contributor Author

Patowhiz commented Jul 9, 2020

@lloyddewit @dannyparsons upon looking this code much further, I realised that the consistent code repetition has reached my threshold of warranting a custom control. In this case, I'm thinking of a custom control that has a ucrInput and a browse button. This control can be implemented in a way that it handles the click events (for both ucrInput and the browse button), processing of file paths, memory functionality, in a more generic way. With this kind of implementation then we can reduce the lines written in these dialogs (and other similar dialogs) by a huge factor (almost down to 1 line of code).

So @lloyddewit as you go through the code please check if my thoughts resonate with yours in regards to this, if they do, I can create an issue about this and have further discussions with the team.

@Patowhiz
Copy link
Contributor Author

Patowhiz commented Jul 9, 2020

On a separate note, I noticed that there are design changes that will conflict with @lilyclements changes in PR #5880, So this PR should not be merged until that PR is merged. I will sort out the conflicts.

@lloyddewit
Copy link
Contributor

@Patowhiz PR #5880 is now merged. I understand that this allows you to prepare this PR for peer review? thanks

@Patowhiz
Copy link
Contributor Author

Patowhiz commented Aug 7, 2020

@lloyddewit thanks for the notification. Yes I have resolved the conflicts. This is ready for your review. Please also take a look at my comment. Thank you

@lloyddewit @dannyparsons upon looking this code much further, I realised that the consistent code repetition has reached my threshold of warranting a custom control. In this case, I'm thinking of a custom control that has a ucrInput and a browse button. This control can be implemented in a way that it handles the click events (for both ucrInput and the browse button), processing of file paths, memory functionality, in a more generic way. With this kind of implementation then we can reduce the lines written in these dialogs (and other similar dialogs) by a huge factor (almost down to 1 line of code).

So @lloyddewit as you go through the code please check if my thoughts resonate with yours in regards to this, if they do, I can create an issue about this and have further discussions with the team.

@lloyddewit
Copy link
Contributor

@Patowhiz yes, good suggestion. There is a lot of repeated code across the 5 dialog boxes. I agree that the best solution would be to create a new custom control (repetition could be handled in other ways but a new ucr would be the most consistent with R-Instat's current approach).
The question is do we make the new ucr as part of this PR or do we raise a new issue for it? If you think that you could complete this PR within 2 weeks with a new ucr (and it doesn't impact your other work too much), then it would make this PR more readable and robust. So this would be my preference. If you prefer to spin off the ucr to a new issue then that's also fine.
What's do you recommend?

@Patowhiz Patowhiz mentioned this pull request Aug 10, 2020
@Patowhiz
Copy link
Contributor Author

@lloyddewit will complete the new control by end of today. I've created a new PR #5924 with an empty control which should be merged immediately to avoid conflicts cause by solution changes, I'm adding the functionality code in this PR and will have it ready for review by end of today.

@Patowhiz
Copy link
Contributor Author

Patowhiz commented Aug 10, 2020

@lloyddewit @rdstern this needed more technical work than I thought but it is now ready.
It has the changes that demonstrate how the ucrInputFile control can be used. You can review it once PR #5924 is merged.

@lloyddewit
Copy link
Contributor

@Ivanluv This PR is now ready for your review.

@Patowhiz Thank you for completing this so promptly. I noticed that this PR still contains changes to the .vbproj file. Are these changes still needed now that PR #5924 is merged?

@Patowhiz
Copy link
Contributor Author

@lloyddewit I'm not seeing any change to the '.vbproj' file. Maybe it was cause I had not synchronised the changes from the master. From my end only 17 files are shown to be changed. All of which relate to this dialogs changed by this PR.

@lloyddewit
Copy link
Contributor

@Patowhiz The .vbproj file is now removed from the list. I think it was fixed by the synchronization with master a few minutes ago, thanks

@lloyddewit
Copy link
Contributor

@Ivanluv Please could you review this PR? thanks

@Ivanluv
Copy link
Contributor

Ivanluv commented Aug 18, 2020

@lloyddewit am okay with these changes

@lloyddewit
Copy link
Contributor

@rdstern Please could you test? thanks

Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

It seems to work. One small additional item for @Patowhiz is in the export dialogue. It does remember the directory, but not the type of data. So the default is a csv file. I saved as excel, but each time I return it is back to csv. Could it please also remember the last type of file.
The save seems fine.

@Patowhiz
Copy link
Contributor Author

@rdstern thanks for the observation. I have now added that feature.

Copy link
Contributor

@lloyddewit lloyddewit left a comment

Choose a reason for hiding this comment

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

@Patowhiz This looks excellent. I especially liked the clarity/consistency between the different dialogs, the Using statement and the XML comments.
I only have some small suggestions and questions. If you can accept/reject these then I can approve.
Thanks

instat/dlgExportGraphAsImage.vb Show resolved Hide resolved
instat/ucrFilePath.vb Outdated Show resolved Hide resolved
instat/dlgExportDataset.vb Outdated Show resolved Hide resolved
instat/dlgExportGraphAsImage.vb Outdated Show resolved Hide resolved
instat/dlgExportRObjects.vb Outdated Show resolved Hide resolved
instat/dlgExportRWorkspace.vb Outdated Show resolved Hide resolved
instat/ucrFilePath.vb Outdated Show resolved Hide resolved
instat/ucrFilePath.vb Outdated Show resolved Hide resolved
instat/ucrFilePath.vb Outdated Show resolved Hide resolved
@lloyddewit
Copy link
Contributor

lloyddewit commented Nov 15, 2020

@rdstern please could you approve? thanks
@dannyparsons I will not merge this until you've also had a chance to review/approve.

rdstern
rdstern previously approved these changes Nov 17, 2020
Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

ok by me

@lloyddewit
Copy link
Contributor

lloyddewit commented Nov 18, 2020

@Patowhiz Please could you resolve the code conflict so that we can merge? thanks
@dannyparsons You asked to review this PR before it was merged

@Patowhiz Patowhiz dismissed stale reviews from rdstern and lloyddewit via d44c740 December 2, 2020 08:44
@Patowhiz
Copy link
Contributor Author

Patowhiz commented Dec 2, 2020

@lloyddewit I have resolved the conflicts. It's now ready.

lloyddewit
lloyddewit previously approved these changes Dec 2, 2020
@lloyddewit
Copy link
Contributor

@dannyparsons please could you review (you asked to review this before it was merged)? thanks

rdstern
rdstern previously approved these changes Jan 4, 2021
@lloyddewit
Copy link
Contributor

@dannyparsons Please could you review? (you asked to review this before it was merged), thanks

@Patowhiz Patowhiz dismissed stale reviews from rdstern and lloyddewit via b4ec475 January 18, 2021 13:19
@Patowhiz
Copy link
Contributor Author

@rdstern as discussed with @volloholic below is the screenshot of the changes made to the export dialog. The file type is now enforced through the combobox, which is now always visible.
@Wycklife you may use these changes for the video production in regards to exporting the multiple data frames imported from climsoft.
@lloyddewit @rdstern this is now ready for review.

Single file selected
screenshot 1

Multiple files Selected
export multiple files

@lloyddewit
Copy link
Contributor

@volloholic @dannyparsons Do these new changes resolve your outstanding comments? Would you still like to review, or may @rdstern and I approve and merge?
thanks for clarifying

@Patowhiz
Copy link
Contributor Author

@rdstern did you manage to test the new design changes proposed by @volloholic
@lloyddewit the new changes address design changes requested by @volloholic after he had discussions with @dannyparsons. These new design changes affect the export dialog only.

Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

@Patowhiz this is looking good and is an important improvement for the next update. Two trivial suggestions.
a) Can the file types field be wider, so most of the types are shown completely.
b) Then, when I was exporting multiple files (e.g. csv) it didn't let me choose the names. I could only choose the directory. I suggest we should say that. So, when there are multiple files and the full set of export options is provided, then the label changes to Export Directory: Perhaps the Export moves to the line above, so the directory name field stays the same size?

@Patowhiz
Copy link
Contributor Author

@rdstern I have addressed item (a).
In regards to item (b) I think it's a standard way in most software packages when it comes to exporting multiple files, users by default expect to select a directory into which they files will be placed into, and also expectation naming the files. I have no strong reservations from making the change other than it is an extra set of functionality that I think is not really necessary(since its normal in regards to user expectations). If others feel that this change is important, I can add it.

Capture

Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

@Patowhiz I am happy now and would really like to see this feature merged. Over to @lloyddewit

Copy link
Contributor

@lloyddewit lloyddewit left a comment

Choose a reason for hiding this comment

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

I reviewed the commits since my last approval

@lloyddewit lloyddewit changed the title Adding memory of directory and filename to export & save dialogs Added memory of directory and filename to export & save dialogs Mar 1, 2021
@lloyddewit lloyddewit merged commit a56c2d2 into IDEMSInternational:master Mar 1, 2021
@Patowhiz Patowhiz deleted the SaveDialogs branch April 12, 2021 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants