Some fixes for table join #452

Merged
merged 3 commits into from Apr 1, 2013

Conversation

Projects
None yet
5 participants
@minorua
Contributor

minorua commented Mar 2, 2013

Fix

  • shift of joined fields (7068-1)
  • restoring joined fields from project files (7068-2)

Also fix

  • refreshing field names after encoding is changed (6840)
@wonder-sk

This comment has been minimized.

Show comment Hide comment
@wonder-sk

wonder-sk Mar 3, 2013

Member

@minorua thanks for the fixes. Regarding the implementation, I think that updateFields() method should not become a part of public interface of QgsVectorLayer, instead it should be called only internally when the update is necessary. This function is really just an implementation detail that other classes should not need to care about.

The refresh of field names should be done in a different way (again to avoid calling updateFields() from outside). I would suggest adding a "fieldsChanged" signal to QgsVectorDataProvider. It would be emitted when the encoding has been changed and later could be used also for other occasions where the provider needs to tell layer that its fields have been changed. QgsVectorLayer should have a private slot that would trigger the update of fields (maybe directly updateFields() could become a private slot).

Could you please update your pull request?

Member

wonder-sk commented Mar 3, 2013

@minorua thanks for the fixes. Regarding the implementation, I think that updateFields() method should not become a part of public interface of QgsVectorLayer, instead it should be called only internally when the update is necessary. This function is really just an implementation detail that other classes should not need to care about.

The refresh of field names should be done in a different way (again to avoid calling updateFields() from outside). I would suggest adding a "fieldsChanged" signal to QgsVectorDataProvider. It would be emitted when the encoding has been changed and later could be used also for other occasions where the provider needs to tell layer that its fields have been changed. QgsVectorLayer should have a private slot that would trigger the update of fields (maybe directly updateFields() could become a private slot).

Could you please update your pull request?

@minorua

This comment has been minimized.

Show comment Hide comment
@minorua

minorua Mar 3, 2013

Contributor

@wonder-sk Thank you for your advice. I realized that my idea was a bit shortsighted.
I don't want to dirty your elegant design by my lack of technical abilities. For a quick fix, it is undesirable to keep you waiting for my inexperienced job to finish. So, please allow me to withdraw this request.

Contributor

minorua commented Mar 3, 2013

@wonder-sk Thank you for your advice. I realized that my idea was a bit shortsighted.
I don't want to dirty your elegant design by my lack of technical abilities. For a quick fix, it is undesirable to keep you waiting for my inexperienced job to finish. So, please allow me to withdraw this request.

@minorua minorua closed this Mar 3, 2013

@minorua

This comment has been minimized.

Show comment Hide comment
@minorua

minorua Mar 5, 2013

Contributor

Updated commits with new ones (old ones were moved to here).
I agreed to add fieldsChanged signal, but I could not find a good way to avoid making updateFields public, to update fieldnames after all layers loaded. If there is a better way, dismissing this request is fine for me.

Contributor

minorua commented Mar 5, 2013

Updated commits with new ones (old ones were moved to here).
I agreed to add fieldsChanged signal, but I could not find a good way to avoid making updateFields public, to update fieldnames after all layers loaded. If there is a better way, dismissing this request is fine for me.

@minorua minorua reopened this Mar 5, 2013

@nirvn

This comment has been minimized.

Show comment Hide comment
@nirvn

nirvn Mar 5, 2013

Contributor

@minorua , @wonder-sk , I've filed http://hub.qgis.org/issues/7271 yesterday before seeing work done here. It might be that your work will fix the underlying issue(s) raised in issue 7271. Might be good for you to test out patched build for the problems raised in 7271.

Contributor

nirvn commented Mar 5, 2013

@minorua , @wonder-sk , I've filed http://hub.qgis.org/issues/7271 yesterday before seeing work done here. It might be that your work will fix the underlying issue(s) raised in issue 7271. Might be good for you to test out patched build for the problems raised in 7271.

@minorua

This comment has been minimized.

Show comment Hide comment
@minorua

minorua Mar 5, 2013

Contributor

@nirvn Additional fix seems to be necessary for 7271.

Contributor

minorua commented Mar 5, 2013

@nirvn Additional fix seems to be necessary for 7271.

@nirvn

This comment has been minimized.

Show comment Hide comment
@nirvn

nirvn Mar 5, 2013

Contributor

@minorua , oh :/ was hoping it wouldn't require additional efforts. the two problems raised there - namely column index shift when sorting and when doing symbology classification - are pretty big. The classification one is especially problematic as a user might not be aware he/she is classifying with data from the wrong column.

Contributor

nirvn commented Mar 5, 2013

@minorua , oh :/ was hoping it wouldn't require additional efforts. the two problems raised there - namely column index shift when sorting and when doing symbology classification - are pretty big. The classification one is especially problematic as a user might not be aware he/she is classifying with data from the wrong column.

@minorua

This comment has been minimized.

Show comment Hide comment
@minorua

minorua Mar 6, 2013

Contributor

The issues should be fixed together with 7271 (part of sort), since they are related.

@nirvn Thanks for your advice!

Contributor

minorua commented Mar 6, 2013

The issues should be fixed together with 7271 (part of sort), since they are related.

@nirvn Thanks for your advice!

@minorua

This comment has been minimized.

Show comment Hide comment
@minorua

minorua Mar 6, 2013

Contributor

Modified commits so that correct indexOffset is calculated even if SubsetOfAttributes is specified in feature request (fix for 7271).

Contributor

minorua commented Mar 6, 2013

Modified commits so that correct indexOffset is calculated even if SubsetOfAttributes is specified in feature request (fix for 7271).

@nirvn

This comment has been minimized.

Show comment Hide comment
@nirvn

nirvn Mar 6, 2013

Contributor

@minorua , no problem, I'm the one who needs to thank you for all of your hard work!

Contributor

nirvn commented Mar 6, 2013

@minorua , no problem, I'm the one who needs to thank you for all of your hard work!

@ghost ghost assigned wonder-sk Mar 7, 2013

@minorua

This comment has been minimized.

Show comment Hide comment
@minorua

minorua Mar 7, 2013

Contributor

@wonder-sk While reviewing commits, I thought that a way of calling updateFields() from QgsVectorLayer::setProviderEncoding() is more simple.
Updated commits (old ones were moved to here). Any opinions welcome.

Contributor

minorua commented Mar 7, 2013

@wonder-sk While reviewing commits, I thought that a way of calling updateFields() from QgsVectorLayer::setProviderEncoding() is more simple.
Updated commits (old ones were moved to here). Any opinions welcome.

@posikifi

This comment has been minimized.

Show comment Hide comment
@posikifi

posikifi Mar 19, 2013

Maybe stupid question: but when this bug correction will be part of master? I need this fix, not only to master but also 1.8.

Maybe stupid question: but when this bug correction will be part of master? I need this fix, not only to master but also 1.8.

@minorua

This comment has been minimized.

Show comment Hide comment
@minorua

minorua Mar 20, 2013

Contributor

Sorry, I've realized that the above commits do not completely fix the issue 7068-2.
joinField/targetField attributes of join element in project file were changed to joinFieldName/targetFieldName. Due to this change, field joins of projects created in 1.8 are not restored in master. Fixing this broken compatibility is required.

Contributor

minorua commented Mar 20, 2013

Sorry, I've realized that the above commits do not completely fix the issue 7068-2.
joinField/targetField attributes of join element in project file were changed to joinFieldName/targetFieldName. Due to this change, field joins of projects created in 1.8 are not restored in master. Fixing this broken compatibility is required.

@posikifi

This comment has been minimized.

Show comment Hide comment
@posikifi

posikifi Mar 20, 2013

I see. I do hope you can fix those sooner than later. Sorry, but I can't really help. Unless testing is helping ;-)

I see. I do hope you can fix those sooner than later. Sorry, but I can't really help. Unless testing is helping ;-)

@minorua

This comment has been minimized.

Show comment Hide comment
@minorua

minorua Mar 21, 2013

Contributor

Added a commit to keep the backward compatibility of table join in project file. Please consider these commits.

Contributor

minorua commented Mar 21, 2013

Added a commit to keep the backward compatibility of table join in project file. Please consider these commits.

@nirvn

This comment has been minimized.

Show comment Hide comment
@nirvn

nirvn Apr 1, 2013

Contributor

@wonder-sk , would be great to merge this when you have some free time. A QGIS 2.0 release with fixed table joins would be fantastic :)

Contributor

nirvn commented Apr 1, 2013

@wonder-sk , would be great to merge this when you have some free time. A QGIS 2.0 release with fixed table joins would be fantastic :)

NathanW2 added a commit that referenced this pull request Apr 1, 2013

Merge pull request #452 from minorua/tablejoin
Some fixes for table join. Merged for wider testing.

@NathanW2 NathanW2 merged commit 5c028f8 into qgis:master Apr 1, 2013

@posikifi

This comment has been minimized.

Show comment Hide comment
@posikifi

posikifi Apr 2, 2013

I briefly tested this. Still some odd features:

  • Some values are only NULL
  • Sometimes last joined column is not available

I hope I can give you sample data and better report tomorrow.

posikifi commented Apr 2, 2013

I briefly tested this. Still some odd features:

  • Some values are only NULL
  • Sometimes last joined column is not available

I hope I can give you sample data and better report tomorrow.

@posikifi

This comment has been minimized.

Show comment Hide comment
@posikifi

posikifi Apr 5, 2013

Well,

It seems that you fix code before I can test ;-) I made some tests with shapefile and csv file: no problems. Then I made thest with Spatialite and csv-file: I can join csv to spatialite layer/view. I can see value in "Attribute table", but when I try to make thematic maps: Qgis will crash.

I found log-message: "FAILURE: Field 4 not found". Field 4 seems to be first joined field: I have 4 field in my Spatialite view.

QGIS code revision 9b83d1a
Windows 7

If you need test data or other debug files, please ask.

P

posikifi commented Apr 5, 2013

Well,

It seems that you fix code before I can test ;-) I made some tests with shapefile and csv file: no problems. Then I made thest with Spatialite and csv-file: I can join csv to spatialite layer/view. I can see value in "Attribute table", but when I try to make thematic maps: Qgis will crash.

I found log-message: "FAILURE: Field 4 not found". Field 4 seems to be first joined field: I have 4 field in my Spatialite view.

QGIS code revision 9b83d1a
Windows 7

If you need test data or other debug files, please ask.

P

@minorua

This comment has been minimized.

Show comment Hide comment
@minorua

minorua Apr 9, 2013

Contributor

@posikifi I have just tested PhilippeDorelon's pull request (#509). It fixes the issue!

Contributor

minorua commented Apr 9, 2013

@posikifi I have just tested PhilippeDorelon's pull request (#509). It fixes the issue!

@posikifi

This comment has been minimized.

Show comment Hide comment
@posikifi

posikifi Apr 9, 2013

@minorua Great, I just wait when it has been merged

posikifi commented Apr 9, 2013

@minorua Great, I just wait when it has been merged

@minorua minorua deleted the minorua:tablejoin branch Apr 26, 2013

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