X-Authentication-Warning: delorie.com: mail set sender to geda-user-bounces using -f X-Recipient: geda-user AT delorie DOT com X-Original-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=prochac.sk; s=default; t=1475876777; bh=FGNB+hNsQZ4R1OTYWVzw8Ya081CrBfif4A3EI162YUk=; h=Subject:To:References:From:Date:In-Reply-To; b=gODWFXaKsuo3ebUjRqpZKcCZ0sdF9FBs7eraR83nA3FP7ptRkH474v62sc3HVPUju Xo0DZ42YcsSU/+7woT5Ug9PHPTlsgp0B92Xicq2a/ee3PVta/VpZP5U2s8F+DiIiYe ff9wGaOoeLLTzBIAADA65cAmB4wEdwuLcgxYLjTU= X-Clacks-Overhead: "GNU Terry Pratchett" Subject: Re: [geda-user] Segfault bug in PCB? To: geda-user AT delorie DOT com References: <1475747077 DOT 18232 DOT 10 DOT camel AT linetec> <5d74ff3f-1470-7745-33b3-385d98cfb961 AT prochac DOT sk> From: "Milan Prochac (milan AT prochac DOT sk) [via geda-user AT delorie DOT com]" Message-ID: Date: Fri, 7 Oct 2016 23:46:05 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/signed; protocol="application/pkcs7-signature"; micalg=sha-256; boundary="------------ms030304080706070803020006" X-Spam-Status: No, score=-101.0 required=5.0 tests=ALL_TRUSTED,HTML_MESSAGE, USER_IN_WHITELIST autolearn=ham autolearn_force=no version=3.4.0 X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on angua.bastl.sk Reply-To: geda-user AT delorie DOT com Errors-To: nobody AT delorie DOT com X-Mailing-List: geda-user AT delorie DOT com X-Unsubscribes-To: listserv AT delorie DOT com Precedence: bulk This is a cryptographically signed message in MIME format. --------------ms030304080706070803020006 Content-Type: multipart/alternative; boundary="------------F02DB2CED30B267F293F5F81" This is a multi-part message in MIME format. --------------F02DB2CED30B267F293F5F81 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable On 07.10.2016 22:31, Chad Parker (parker DOT charles AT gmail DOT com) [via=20 geda-user AT delorie DOT com] wrote: > I'm very much still learning this code, so, thanks for your comments. > > > EraseText also adds the bounding box of text to area to be > redrawn, like any other Erase... functions. But only EraseText > removes the entity from rtree, no other Erase... function does it. > I think that major purpose of EraseText function should be to > invalidate drawing area. Just for code clarity. > > > Then why not just invalidate the drawing area directly and skip the=20 > EraseText function altogether? It makes sense to me that a function=20 > with such a name would do both things, but I see that you're right=20 > that none of the other Erase functions do this, so you're probably=20 > right. Either that, or change all of them to match EraseText... :) If=20 > the Erase functions are always paired with a r_delete_entry (as is the = > case with EraseText), then putting the r_delete_entry in the Erase=20 > functions removes some duplication of code. You put the call in one=20 > place instead of a dozen. I haven't look through enough to see if that = > is also be true for any of the other Erase functions. > It's not always paired - see ChangeTextJoin function. I did not check=20 other (non-text) functions. The change of all Erase functions is far beyond scope of bug fix.=20 Anyway, it would introduce some kind of asymmetry for Erase... / Draw... = function pairs. > > But other functions - like text move, rotate - use r_delete_entry > and r_insert_entry in pairs and r_delete_entry in EraseText is > IMO redundant. > > > Or the calls in move, rotate, etc. are redundant. > > > I did not notice that any text disappearing while undoing size > change... but it disappears after undoing the text creation :) > > > If you delete the call to r_delete_entry in EraseText, then this=20 > doesn't happen. But before applying either solution this was happening = > to me on both my Mac and my Linux VM. So we fixed another bug... > For text objects the Scale parameter is used. It's type of int, > but sizeof(Coord) should be >=3D than sizeof(int). So for time bein= g > it should be OK to accept the type impurity without changing the > data structures. > > > If Coord translates to a long, than if you cast it as an int, would=20 > you get the wrong bytes? If you cast it properly, there is no reason to get wrong byte. But the=20 problem was that original Undo code cast the whole structure to Pintype=20 =2E.. it worked for most types, as they share the first part, but Texttyp= e=20 does not share it, so undo of text size overwrote wrong part of Texttype = structure (possibly, I did not count bytes). Both fixes - one with casting, second one with new structure member -=20 solve the issue. > But it is very easy to extend the Undo data structure by new > "Scale" property to safely store the text Scale value. (done in > new patch) > > > Or change Scale to be type Coord. When I look through global.h I don't = > see too many things declared as ints. > > --Chad > --------------F02DB2CED30B267F293F5F81 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable
I'm very much still learni= ng this code, so, thanks for your comments.


EraseText also= adds the bounding box of text to area to be redrawn, like any other Erase... functions. But only EraseText removes the entity from rtree, no other Erase... function does it. I think that major purpose of EraseText function should be to invalidate drawing area. Just for code clarity.


Then why not just invalidate the drawing area directly and skip the EraseText function altogether? It makes sense to me that a function with such a name would do both things, but I see that you're right that none of the other Erase functions do this, so you're probably right. Either that, or change all of them to match EraseText... :) If the Erase functions are always paired with a r_delete_entry (as is the case with EraseText), then putting the r_delete_entry in the Erase functions removes some duplication of code. You put the call in one place instead of a dozen. I haven't look through enough to see if that is also be true for any of the other Erase functions.
=C2=A0

It's not always paired - see ChangeTextJoin=C2=A0 function. I did not= check other (non-text) functions.

The change of all Erase functions is far beyond scope of bug fix. Anyway, it would introduce some kind of asymmetry for Erase... / Draw... function pairs.


But other functions - like text move, rotate - use r_delete_entry and=C2=A0 r_insert_entry in pairs and r_delete_entry in EraseText is IMO redundant.

Or the calls in move, rotate, etc. are redundant.
=C2=A0

I did not notice that any text disappearing while undoing size change... but it disappears after undoing the text creation :)


If you delete the call to r_delete_entry in EraseText, then this doesn't happen. But before applying either solution this was happening to me on both my Mac and my Linux VM.
=C2=A0

So we fixed another bug...

For text objec= ts the Scale parameter is used.=C2=A0 It's type of int, bu= t sizeof(Coord) should be >=3D than sizeof(int). So fo= r time being it should be OK to accept the type impurity without changing the data structures.


If Coord translates to a long, than if you cast it as an int, would you get the wrong bytes?

If you cast it properly, there is no reason to get wrong byte.=C2=A0 = But the problem was that original Undo code cast the whole structure to Pintype ... it worked for most types, as they share the first part, but Texttype does not share it, so undo of text size overwrote wrong part of Texttype structure (possibly, I did not count bytes).

Both fixes - one with casting, second one with new structure member - solve the issue.

=C2=A0
But it is very= easy to extend the Undo data structure by new "Scale" property to safely store the text Scale value. (done in new patch)


Or change Scale to be type Coord. When I look through global.h I don't see too many things declared as ints.

--Chad


--------------F02DB2CED30B267F293F5F81-- --------------ms030304080706070803020006 Content-Type: application/pkcs7-signature; name="smime.p7s" Content-Transfer-Encoding: base64 Content-Disposition: attachment; filename="smime.p7s" Content-Description: S/MIME Cryptographic Signature MIAGCSqGSIb3DQEHAqCAMIACAQExDzANBglghkgBZQMEAgEFADCABgkqhkiG9w0BBwEAAKCC CtowggTwMIID2KADAgECAhBqqLAGfunD4XYlcEsP4ElzMA0GCSqGSIb3DQEBCwUAMHUxCzAJ BgNVBAYTAklMMRYwFAYDVQQKEw1TdGFydENvbSBMdGQuMSkwJwYDVQQLEyBTdGFydENvbSBD ZXJ0aWZpY2F0aW9uIEF1dGhvcml0eTEjMCEGA1UEAxMaU3RhcnRDb20gQ2xhc3MgMSBDbGll bnQgQ0EwHhcNMTYwMzA3MDEwODU2WhcNMTcwMzA3MDEwODU2WjA8MRkwFwYDVQQDDBBtaWxh bkBwcm9jaGFjLnNrMR8wHQYJKoZIhvcNAQkBFhBtaWxhbkBwcm9jaGFjLnNrMIIBIjANBgkq hkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA0mKGFeH4hukzo8EbR08H7ZFZ1uAI3mTA51yN6C/Q R++4fhe2R6U1yGR2bQeWal47aLFt4bhUEED1cvU4GDT+tzJUeQMSghB1qwrPQfzJueT3ReYA 5uaym6q2tFR9qm5lhSWqac//+mPFThodIz7rMT4a5R2ZnQjBSiEOVP73Sf0jiD7ylNBqvkCd hKFTSt7IkoRCkVE8o7KYEjzEhsSu1JAuCh8rYpz1vajpvO+H3FJyDXbHqCxH4MTmA1PMkObc BPHnsA4NE9xJpghiqJWUyEmcrNSRikLgJY2o0bO2y7AZ8AfXGt1lCkcg2f879JxuswDYaQFH rfPERgaIJ2OCLQIDAQABo4IBszCCAa8wDgYDVR0PAQH/BAQDAgSwMB0GA1UdJQQWMBQGCCsG AQUFBwMCBggrBgEFBQcDBDAJBgNVHRMEAjAAMB0GA1UdDgQWBBQBZgVnt+lqmMtzm6Wfvlmr aFGERzAfBgNVHSMEGDAWgBQkgWw5Yb5JD4+3G0YrySi1J0htaDBvBggrBgEFBQcBAQRjMGEw JAYIKwYBBQUHMAGGGGh0dHA6Ly9vY3NwLnN0YXJ0c3NsLmNvbTA5BggrBgEFBQcwAoYtaHR0 cDovL2FpYS5zdGFydHNzbC5jb20vY2VydHMvc2NhLmNsaWVudDEuY3J0MDgGA1UdHwQxMC8w LaAroCmGJ2h0dHA6Ly9jcmwuc3RhcnRzc2wuY29tL3NjYS1jbGllbnQxLmNybDAbBgNVHREE FDASgRBtaWxhbkBwcm9jaGFjLnNrMCMGA1UdEgQcMBqGGGh0dHA6Ly93d3cuc3RhcnRzc2wu Y29tLzBGBgNVHSAEPzA9MDsGCysGAQQBgbU3AQIEMCwwKgYIKwYBBQUHAgEWHmh0dHA6Ly93 d3cuc3RhcnRzc2wuY29tL3BvbGljeTANBgkqhkiG9w0BAQsFAAOCAQEAcdXDR99mAwWbAilc x4iAlxzxTpAFBwXjDbFUFuLXHtXUQjfQb6isV/A6HGhFfEeloJ/dl0hZNfn6ROken+Pv8UzA sgbpjVQHVx334qK38Fbsoy/L991YYRyJGGl7OJYRs7SY8gJ+kVeC4pN6tdeACvmxdRYyA7r0 AjwhzCOpc449UQO2YQRZ6uzXvo3auQhkFQDCN6Gzn82fr5KRQ26fzUYOphHjkNTUHmYJRbjA QPIJlYSoc9/Gayl3tHscOt8pSXsfJ2alDnD5Ww94E7corjF7kkPdHQvkvZY5Ux72B7eEbSm8 w+FnngjA2Jc7o25oU+weiVOeBGxjo+HlwjaqPzCCBeIwggPKoAMCAQICEGunin0K14jWUQr5 WeTntOEwDQYJKoZIhvcNAQELBQAwfTELMAkGA1UEBhMCSUwxFjAUBgNVBAoTDVN0YXJ0Q29t IEx0ZC4xKzApBgNVBAsTIlNlY3VyZSBEaWdpdGFsIENlcnRpZmljYXRlIFNpZ25pbmcxKTAn BgNVBAMTIFN0YXJ0Q29tIENlcnRpZmljYXRpb24gQXV0aG9yaXR5MB4XDTE1MTIxNjAxMDAw NVoXDTMwMTIxNjAxMDAwNVowdTELMAkGA1UEBhMCSUwxFjAUBgNVBAoTDVN0YXJ0Q29tIEx0 ZC4xKTAnBgNVBAsTIFN0YXJ0Q29tIENlcnRpZmljYXRpb24gQXV0aG9yaXR5MSMwIQYDVQQD ExpTdGFydENvbSBDbGFzcyAxIENsaWVudCBDQTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCC AQoCggEBAL192vfDon2D9luC/dtbX64eG3XAtRmvmCSsu1d52DXsCR58zJQbCtB2/A5uFqNx WacpXGGtTCRk9dEDBlmixEd8QiLkUfvHpJX/xKnmVkS6Iye8wUbYzMsDzgnpazlPg19dnSqf hM+Cevdfa89VLnUztRr2cgmCfyO9Otrh7LJDPG+4D8ZnAqDtVB8MKYJL6QgKyVhhaBc4y3bG WxKyXEtx7QIZZGxPwSkzK3WIN+VKNdkiwTubW5PIdopmykwvIjLPqbJK7yPwFZYekKE015Os W6FV+s4DIM8UlVS8pkIsoGGJtMuWjLL4tq2hYQuuN0jhrxK1ljz50hH23gA9cbMCAwEAAaOC AWQwggFgMA4GA1UdDwEB/wQEAwIBBjAdBgNVHSUEFjAUBggrBgEFBQcDAgYIKwYBBQUHAwQw EgYDVR0TAQH/BAgwBgEB/wIBADAyBgNVHR8EKzApMCegJaAjhiFodHRwOi8vY3JsLnN0YXJ0 c3NsLmNvbS9zZnNjYS5jcmwwZgYIKwYBBQUHAQEEWjBYMCQGCCsGAQUFBzABhhhodHRwOi8v b2NzcC5zdGFydHNzbC5jb20wMAYIKwYBBQUHMAKGJGh0dHA6Ly9haWEuc3RhcnRzc2wuY29t L2NlcnRzL2NhLmNydDAdBgNVHQ4EFgQUJIFsOWG+SQ+PtxtGK8kotSdIbWgwHwYDVR0jBBgw FoAUTgvvGqRAW6UXaYcwyjRoQ9BBrvIwPwYDVR0gBDgwNjA0BgRVHSAAMCwwKgYIKwYBBQUH AgEWHmh0dHA6Ly93d3cuc3RhcnRzc2wuY29tL3BvbGljeTANBgkqhkiG9w0BAQsFAAOCAgEA i+P3h+wBi4StDwECW5zhIycjBL008HACblIf26HY0JdOruKbrWDsXUsiI0j/7Crft9S5oxvP iDtVqspBOB/y5uzSns1lZwh7sG96bYBZpcGzGxpFNjDmQbcM3yl3WFIRS4WhNrsOY14V7y2I rUGsvetsD+bjyOngCIVeC/GmsmtbuLOzJ606tEc9uRbhjTu/b0x2Fo+/e7UkQvKzNeo7OMhi jixaULyINBfCBJb+e29bLafgu6JqjOUJ9eXXj20p6q/CW+uVrZiSW57+q5an2P2i7hP85jQJ cy5j4HzA0rSiF3YPhKGAWUxKPMAVGgcYoXzWydOvZ3UDsTDTagXpRDIKQLZo02wrlxY6iMFq vlzsemVf1odhQJmi7Eh5TbxI40kDGcBOBHhwnaOumZhLP+SWJQnjpLpSlUOj95uf1zo9oz9e 0NgIJoz/tdfrBzez76xtDsK0KfUDHt1/q59BvDI7RX6gVr0fQoCyMczNzCTcRXYHY0tq2J0o T+bsb6sH2b4WVWAiJKnSYaWDjdA70qHX4mq9MIjO/ZskmSY8wtAk24orAc0vwXgYanqNsBX5 Yv4sN4Z9VyrwMdLcusP7HJgRdAGKpkR2I9U4zEsNJQJewM7S4Jalo1DyPrLpL2nTET8ZrSl5 Utp1UeGp/2deoprGevfnxWB+vHNQiu85o6MxggPMMIIDyAIBATCBiTB1MQswCQYDVQQGEwJJ TDEWMBQGA1UEChMNU3RhcnRDb20gTHRkLjEpMCcGA1UECxMgU3RhcnRDb20gQ2VydGlmaWNh dGlvbiBBdXRob3JpdHkxIzAhBgNVBAMTGlN0YXJ0Q29tIENsYXNzIDEgQ2xpZW50IENBAhBq qLAGfunD4XYlcEsP4ElzMA0GCWCGSAFlAwQCAQUAoIICEzAYBgkqhkiG9w0BCQMxCwYJKoZI hvcNAQcBMBwGCSqGSIb3DQEJBTEPFw0xNjEwMDcyMTQ2MDVaMC8GCSqGSIb3DQEJBDEiBCCg fw831hxOOllpDcIGljGehYhzaDql3F28EAKqOqKDozBsBgkqhkiG9w0BCQ8xXzBdMAsGCWCG SAFlAwQBKjALBglghkgBZQMEAQIwCgYIKoZIhvcNAwcwDgYIKoZIhvcNAwICAgCAMA0GCCqG SIb3DQMCAgFAMAcGBSsOAwIHMA0GCCqGSIb3DQMCAgEoMIGaBgkrBgEEAYI3EAQxgYwwgYkw dTELMAkGA1UEBhMCSUwxFjAUBgNVBAoTDVN0YXJ0Q29tIEx0ZC4xKTAnBgNVBAsTIFN0YXJ0 Q29tIENlcnRpZmljYXRpb24gQXV0aG9yaXR5MSMwIQYDVQQDExpTdGFydENvbSBDbGFzcyAx IENsaWVudCBDQQIQaqiwBn7pw+F2JXBLD+BJczCBnAYLKoZIhvcNAQkQAgsxgYyggYkwdTEL MAkGA1UEBhMCSUwxFjAUBgNVBAoTDVN0YXJ0Q29tIEx0ZC4xKTAnBgNVBAsTIFN0YXJ0Q29t IENlcnRpZmljYXRpb24gQXV0aG9yaXR5MSMwIQYDVQQDExpTdGFydENvbSBDbGFzcyAxIENs aWVudCBDQQIQaqiwBn7pw+F2JXBLD+BJczANBgkqhkiG9w0BAQEFAASCAQCObZDyJTtIVB65 ppKbp9aqzKEIGsRCKZMvlifpO14Ja2OdvK+pLvSpmr6t+EKh02AhLusPlbhgUhRtkL4op+VA KOY+BxhAvVDiFmoMWaULydUpPWDIK6+v2TeM6ddZZ9S1f1aFEr8mxK3CFCKv4YQaNzZ0kTov nUGSWkzebwaodocKDFrUw17xa05qXadCHffOkQS5enfuAm2CDl1o7aA80VEl5upUNIZjhQL7 anGMDGETD0pAmN9RhmmgmEpYwLhVswVd3byuSrO2247xZ9s39BJNTnFvJG1OmNqIzyxF8qls Nq/qf8McHpH86Q62h6Y/j/nkHJeZHy1hYcqPXqYXAAAAAAAA --------------ms030304080706070803020006--