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

construct shapefiles in memory #890

Merged
merged 62 commits into from Apr 3, 2017
Merged

construct shapefiles in memory #890

merged 62 commits into from Apr 3, 2017

Conversation

@donogst
Copy link
Contributor

@donogst donogst commented Nov 20, 2016

Fixes #885
dotspatial.data.tests/zipexporttests.cs shows example of how to create the archive as well as testing that the contents of the archive match those of a regular save operation.
I think it has squashed correctly if for some reason it has not worked, let me know and I will try again

Checklist

  • [ y] I have included examples or tests
  • [ y] I have updated the change log
  • [ y] I am listed in the CONTRIBUTORS file
  • [ y] I have cleaned up the commit history (use rebase and squash)

Changes proposed in this pull request:

  • construct shapefiles in memory only packaged in zip archive for download
  • refactor of shapefile classes
  • shape export tests
@mogikanin
Copy link
Contributor

@mogikanin mogikanin commented Nov 20, 2016

We can't use 3-rd party dependencies in DotSpatial core libraries.
But we can split your changes into several parts:

  1. Refactorings for support writing into streams.
  2. Separate plugin\assembly with ExportZipFile(string ShapefileName) functionality. We can include it into DS repo as plugin, and later publish it as separate assembly at nuget.

@donogst
Copy link
Contributor Author

@donogst donogst commented Nov 20, 2016

Hi, ok, I had seen that library elsewhere but obviously not in core.
Another option would be to upgrade to framework 4.5 which has native
support but that may be a large change for a small feature unless it is
already planned to use 4.5
On 20 Nov 2016 08:49, "Max Miroshnikov" notifications@github.com wrote:

We can't use 3-rd party dependencies in DotSpatial core libraries.
But we can split your changes into several parts:

  1. Refactorings for support writing into streams.
  2. Separate plugin\assembly with ExportZipFile(string ShapefileName)
    functionality. We can include it into DS repo as plugin, and later publish
    it as separate assembly at nuget.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#890 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFFvJ1OgKMWMs6_MPpkj_RZceaKqR1Zbks5rAAmggaJpZM4K3aeu
.

@mogikanin
Copy link
Contributor

@mogikanin mogikanin commented Dec 15, 2016

@donogst , would you like to refactor your changes, because in current state we can't merge PR?

@donogst
Copy link
Contributor Author

@donogst donogst commented Dec 15, 2016

SteveDonoghuePro and others added 7 commits Dec 15, 2016
… in legend for all layers with LegendTextReadOnly == true

- set Browsable for LegendText  to false because otherwise users could change the LegendText in Properties window
- closes #750
- moved DetailedPolygonSymbolControl designer code to designer file
- load hatch style combobox on configure so it's filled before the first style is selected
- color/opacity choosers of hatch and simple pattern now work like outline color/opacity chooser
- closes #851
@donogst
Copy link
Contributor Author

@donogst donogst commented Dec 29, 2016

@mogikanin
Copy link
Contributor

@mogikanin mogikanin commented Jan 8, 2017

@donogst I think you forgot to submit it as pull request. I see it in your branches, but not in pull requests.

@donogst
Copy link
Contributor Author

@donogst donogst commented Jan 8, 2017

@mogikanin
Copy link
Contributor

@mogikanin mogikanin commented Jan 18, 2017

Steve, please do a few fixes:

  1. Remove Contributors from .gitignore
  2. Remove dependencies of DotNetZip, NUnit.VisualStudio.TestAdapter from DotSpatial.Data.Tests and DotSpatial.Data

mkaring and others added 6 commits Oct 28, 2016
Improved the loading of the different data types according to the dBASE
specifications.

Also added a test to verify some critical cases.
Removed some dummy xml documentation.
@donogst
Copy link
Contributor Author

@donogst donogst commented Jan 29, 2017

@mogikanin
Copy link
Contributor

@mogikanin mogikanin commented Mar 12, 2017

@jany-tenaj please take a look into this PR too.

@jany-tenaj
Copy link
Contributor

@jany-tenaj jany-tenaj commented Mar 16, 2017

@donogst Please correct the comments that were added.

  1. Use full sentences if possible and correct spelling.
  2. Explain clearly what the functions do, e.g. LineShapefile.SetHeaderShapeType comments that it returns something but it's a void function
  3. If the function has parameters or return values please add comments for them, too.
  4. Why are there two blanks after the . in the comment of Shapefile.WriteFileLength? I've seen this in other places, too, but still don't understand it.

In PointShapefile.ExportShapefilePackage decide whether you want to remove the UpdateAttributes(); as indicated by your comment // remove this call to update attributes //? or remove the comment. Who is supposed to decide this if not you?

@jany-tenaj
Copy link
Contributor

@jany-tenaj jany-tenaj commented Mar 29, 2017

@mogikanin While trying to fix the Export to work for !IndexMode I found the following thing.

We use BufferedBinaryWriter when not in IndexMode for Line, Polygon and Multipoint shapefiles.

We use FileStream when not in IndexMode for point shapefiles and when in IndexMode for all shapefiles.

Do you have an idea why we're using BufferedBinaryWriter only in the first 3 cases?

I'd like to change those 3 case to work like the others. Do you see any reason against doing that?

@donogst
Copy link
Contributor Author

@donogst donogst commented Mar 29, 2017

@jany-tenaj
Copy link
Contributor

@jany-tenaj jany-tenaj commented Mar 29, 2017

@donogst Nope it's used in master, too.

@mogikanin
Copy link
Contributor

@mogikanin mogikanin commented Mar 29, 2017

@jany-tenaj I think we should use standard FileStream in all cases, and do not use these custom BufferedBinaryWriter\BufferedBinaryReader. I don't see in them any sense, because FileStream itself uses buffering and we can specify bufferSize via 'FileStream' constructor,

jany-tenaj added 2 commits Apr 2, 2017
- fixed export for !IndexMode
- added export tests for !IndexMode
- added/corrected comments for functions that were created/changed for export
- moved inMemoryStream into AttributeTable.ExportDbfToStream because it doesn't seem to be used somewhere else
- moved error messages to resource
- moved ExportShapefilePackage because it works the same now for all shapefile types
- changed write functions of LineShapefile, to reuse them in MultpointShapefile because the code was almost exactly the same
- in PopulateShpAndShxStreamsNotIndexed write directly to the stream instead of using BufferedBinaryReader
- changed code so NullShapes are not only recognized for polygon / line shapes when in IndexMode, but also in !IndexMode and for points and multipoints too
- added/changed NullShape export tests
- fixed some ReSharper warnings
@jany-tenaj
Copy link
Contributor

@jany-tenaj jany-tenaj commented Apr 2, 2017

@mogikanin Shapefile class seems to be meant as base class for the specific shapefile types. Why does it have to be instantiateable? We already have FeatureSet that can be instantiated without specifying FeatureType or ShapeType of the features that will be added.

@mogikanin
Copy link
Contributor

@mogikanin mogikanin commented Apr 2, 2017

Yes, we can make it abstract.

@jany-tenaj
Copy link
Contributor

@jany-tenaj jany-tenaj commented Apr 2, 2017

@donogst Please check everything still works for you after my changes.

@mogikanin If you don't see any other problems I think we can merge now.

@mogikanin
Copy link
Contributor

@mogikanin mogikanin commented Apr 3, 2017

@jany-tenaj I found small issue. After fixing it please merge PR.

@jany-tenaj
Copy link
Contributor

@jany-tenaj jany-tenaj commented Apr 3, 2017

@mogikanin Are you going to fix it or tell me what the issue is?

@mogikanin
Copy link
Contributor

@mogikanin mogikanin commented Apr 3, 2017

You should see it now. It is in "review".

@jany-tenaj
Copy link
Contributor

@jany-tenaj jany-tenaj commented Apr 3, 2017

Sorry but I don't see any review.

try
{
using (var inMemoryStream = new MemoryStream())
using (_writer = new BinaryWriter(inMemoryStream))

This comment has been minimized.

@mogikanin

mogikanin Apr 3, 2017
Contributor

Here should be used local variable, not field _writer.

This comment has been minimized.

@jany-tenaj

jany-tenaj Apr 3, 2017
Contributor

We have to use _writer because it is used inside WriteTable();

This comment has been minimized.

@mogikanin

mogikanin Apr 3, 2017
Contributor

Ah, ok. I think later we should refactor this class, because this _writer initialized in several places, and logic is messy.

@jany-tenaj jany-tenaj merged commit 56770cb into DotSpatial:master Apr 3, 2017
2 checks passed
2 checks passed
CodeFactor 62 issues fixed. 34 issues found.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants