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

Propose to depreciate childWithName+ #207

Closed
labordep opened this issue Feb 3, 2023 · 20 comments
Closed

Propose to depreciate childWithName+ #207

labordep opened this issue Feb 3, 2023 · 20 comments
Assignees

Comments

@labordep
Copy link
Contributor

labordep commented Feb 3, 2023

Hi @tinchodias,
I propose to depreciate all BlElement>>childWithName+ methods because of multiples points :

  • There are BlElement>>childWithId+ methods which are doing the same things
  • The property is elementId
  • Name is not the most common term to speak about an identification of a graphical element (but this point is subjective from my experience) and we prefer id which is most operationnal
  • There are a lot of methods in BlElement and a little cleaning is good :)

Thanks for your feedback.

@plantec
Copy link
Collaborator

plantec commented Feb 6, 2023

yes #childNamed: and #childWidthId: seem identical

@tinchodias
Copy link
Collaborator

I think your proposal is right.
Do you plan a PR?
there is a way to deprecate that also rewrites senders on runtime. This could be good in the case they are identical.

@labordep
Copy link
Contributor Author

labordep commented Feb 7, 2023

Ok no problem to do a PR.
Can you write here the way to depreciate and rewrite ? Thanks :)

@labordep labordep self-assigned this Feb 7, 2023
@labordep
Copy link
Contributor Author

@tinchodias I have tried to refactor some methods here : https://github.com/labordep/Bloc
But there is a problem and I don't know if this is a Pharo problem or a my bad.
I have writted some tests, for example BlElementTest>>testId and when I launch them (or others Bloc tests), that's freeze my Pharo image.
Can you check if you have same problem ? Thanks :)

@tinchodias
Copy link
Collaborator

Hi @labordep I of got the same image freeze!

My steps were: clone your repo on my Mac and run the build.sh script to install it.

Then I started to place halts and ended placing one at the first line of BlElementTest>>#setUp.

There first I noticed an exception in a debugger (wantsSteps blah blah) in the screenshot below, and I went to the Preferences to disable dialog warnings checkbox (I suspected this can help, but wasn't sure at all):
Screenshot 2023-02-15 at 17 16 43

Then run again the test and the error was different:
Screenshot 2023-02-15 at 17 17 19

At that point I realized the rewriting expression may require the argument (see inline comment in the repo). And yes, it was required. So now the test runs without freezing, but it fails: the id returned by the BlElement is not the Symbol but an BlElementId I think. The test fail message is confusing because the printString of the BlElementId is exactly like the Symbol. I'm not sure there what is the good solution, I let it to the author ;)

@labordep
Copy link
Contributor Author

Thanks @tinchodias I will check that :)

@labordep
Copy link
Contributor Author

I have created an issue for Pharo about this problem : pharo-project/pharo#12791

@MarcusDenker
Copy link

The issue tracker entry for Pharo uses #name on Class.. there is no way that this can work, as #name is called again in the debugger (or the exception).

@MarcusDenker
Copy link

wantSteps might be related to

pharo-project/pharo#12782

@MarcusDenker
Copy link

If I use this on a simple example

  • I do see the wantSteps problem, we will fix it (but just press proceed)
  • SyntaxError debugger opens, no problem. As expected, I close it, I can continue to work

It might be that you called this in a situation where the method with the error is called in a loop?

@labordep
Copy link
Contributor Author

labordep commented Feb 20, 2023

Possible, but how can I stop the loop to check that ?

@MarcusDenker
Copy link

You can't... it will allocate so much memory so fast that it just freezes.
Normally you can stop a recursiomn if you are fast. But this recursion tiggeres itself again with the cmd-dot: it wants to open a debugger, and that need #name, too.

Adding a DNU in #name is just not a good thing to do.

@labordep
Copy link
Contributor Author

Thanks @MarcusDenker :)
If I understand, this code :

containerName
	<return: #Symbol or: nil>

	self
		deprecated: 'Use #id instead.'
		transformWith: '`@receiver containerName' -> '`@receiver id'.

	^ self id asSymbol

is not a good idea because id is used a lot by the system. This is a exception case, for others we will haven't this problems (except for very used methods names).

@MarcusDenker
Copy link

if you do it with a method that has lots and lots of calls (in a loop, maybe even another process), and you add an error there, you will have problems... this got a bit confusing as you used #name in Class in the other issue tracker: that method is better not to be touched, as it is used to print the exception when the DNU happens.

In the case of containerName I guess it is similar, a method that is used a lot while the system is running.

@labordep
Copy link
Contributor Author

Yes I think container Name and id are very used, but normally it's should be not (this is another problem).

@tinchodias
Copy link
Collaborator

Hi, what happened with the associated code change?

@tinchodias
Copy link
Collaborator

Looking at usages of containerName and containerName:, I guess they are legacy from another time where they made sense, but now should really be deprecated and substituted.

@plantec
Copy link
Collaborator

plantec commented Apr 6, 2023

ok :)

@tinchodias
Copy link
Collaborator

tinchodias commented Nov 27, 2023

Was this issue closed by #207 ?


EDIT: I meant "Was this issue closed by #297 ?"

@labordep
Copy link
Contributor Author

Was this issue closed by #207 ?

Yes!

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

No branches or pull requests

4 participants