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

Finish chapter "Managing windows" #79

Closed
3 tasks done
koendehondt opened this issue Apr 19, 2024 · 12 comments · Fixed by #102
Closed
3 tasks done

Finish chapter "Managing windows" #79

koendehondt opened this issue Apr 19, 2024 · 12 comments · Fixed by #102
Assignees
Milestone

Comments

@koendehondt
Copy link
Collaborator

koendehondt commented Apr 19, 2024

  • Check all code examples and images

Open issues from #50:

Section on setting the icon of a window

  • The explanation and the example are wrong. Sending windowIcon: has no effect. SpWindowPresenter>>#taskbarIcon is deprecated (it is in the TOREMOVE protocol). Setting the icon is not implemented in Spec 2. I think this incompleteness should be fixed in Pharo 12, otherwise this section has to be removed.

Section on setting the about text

  • SpWindowPresenter>>#aboutText: is in the TOREMOVE protocol, so this section is not up-to-date. Similar to the section on the icon, I think this issue should be addressed in Pharo 12.
@koendehondt koendehondt added this to the Book release milestone Apr 19, 2024
@koendehondt koendehondt self-assigned this May 18, 2024
@koendehondt
Copy link
Collaborator Author

@Ducasse We have a problem with the explanation in section "Preventing window close". That section states that implementing:

WindowExamplePresenter >> okToChange

   ^ false

makes the window uncloseable. That is not correct. requestWindowClose has to be implemented instead. That is clear fromSystemWindow>>#closeBoxHit:

closeBoxHit
	"The user clicked on the close-box control in the window title.
	For Mac users only, the Mac convention of option-click-on-close-box is obeyed if the mac option key is down.
	If we have a modal child then don't delete.
	Play the close sound now since this is the only time we know that the close is user-initiated."

	self allowedToClose ifFalse: [^self].
	self playCloseSound.
	self close

and SpWindow>>#allowedToClose:

allowedToClose

	super allowedToClose ifFalse: [ ^ false ].
	self model ifNil: [ ^ true ].
	^ self model askOkToClose 
		ifTrue: [ self model requestWindowClose ]
		ifFalse: [ true ].

The latter sends requestWindowClose, but its implementationSpPresenter>>#requestWindowClose is categorized in the TOREMOVE protocol:

requestWindowClose

	"returns <true> if the user is allowed to close the window. Useful if you want to ask user if he wants to save the changed content etc."
	
	^ true

So it is unclear to me whether using requestWindowClose is a good idea. Is the protocol name incorrect? Or are there plans with Spec that I am not aware of?

Until we have resolved this issue, I will put this in the book because that is needed to support the explanation in this chapter.

WindowExamplePresenter >> requestWindowClose

   ^ false

@koendehondt
Copy link
Collaborator Author

Forgot to add: there are no senders of requestWindowClose in the image. Strange.

@koendehondt
Copy link
Collaborator Author

@Ducasse Another issue. The same section uses self askOkToClose: true, which is implemented as:

askOkToClose: aBoolean

	askOkToClose := aBoolean

but the implementation of the accessor SpPresenter>>#askOkToClose is:

askOkToClose
	"DO NOT USE
	With Spec 2, SpPresenter was refactored to move all window management to WindowPresenter.
	From now on, if you want to interact with a window you need to:
	- Implement #initializeWindow: method (#initializeDialog: for dialogs) to manage window elements before the presenter is opened
	- Use the method #window or #withWindowDo: to interact with windows after it has been opened.
	
	This method cannot be deprecated because during a transition phase we keep this mecanism. "

	^ askOkToClose

There is something fishy about all the window closing behavior.

@Ducasse
Copy link
Member

Ducasse commented May 18, 2024

I have no idea :(
We should ask @estebanlm else we remove it from the book because I do not want to document something that will be removed.

@estebanlm
Copy link
Contributor

yeah, do not use it :)

@estebanlm
Copy link
Contributor

estebanlm commented May 18, 2024

there is an event (whenWillCloseDo:) to achieve the same now).
it reaceives an event that can denyClose if needed.

@koendehondt
Copy link
Collaborator Author

About

SpWindowPresenter>>#aboutText: is in the TOREMOVE protocol, so this section is not up-to-date. Similar to the section on the icon, I think this issue should be addressed in Pharo 12.

I think SpWindowPresenter>>#aboutText: is in the TOREMOVE protocol by mistake because there is no other way to set the about text of a window. @estebanlm could you explain why the method is in the TOREMOVE protocol? Are there plans with the about text that are not documented in the code?

@koendehondt
Copy link
Collaborator Author

About

The explanation and the example are wrong. Sending windowIcon: has no effect. SpWindowPresenter>>#taskbarIcon is deprecated (it is in the TOREMOVE protocol). Setting the icon is not implemented in Spec 2. I think this incompleteness should be fixed in Pharo 12, otherwise this section has to be removed.

After careful inspection of the code, I conclude that setting the taskbar icon for a window is not implemented correctly. Look at SpPresenter>>#initializeWindow::

initializeWindow: aWindowPresenter
	"override this to set window values before opening. 
	 You may want to add a menu, a toolbar or a statusbar"

	"IMPORTANT: Please ovirride this method and set yourself the informations you want in your window.
	The content of this method is here to help the transition between Spec 1 and 2.
	In the next Spec version the content of this method will be removed and it will do nothing by default because the goal is to remove the management of all of those informations from Composable to put them in WindowPresenter."

	aWindowPresenter
		title: self title;
		initialExtent: self initialExtent;
		windowIcon: self windowIcon

As stated before, sending windowIcon: self windowIcon does not have an effect. The method is not used anymore.

SystemWindow>>#taskbarTask does this:

taskbarTask
	"Answer a taskbar task for the receiver.
	Answer nil if not required."

	(self valueOfProperty: #noTaskbarTask ifAbsent: [false]) ifTrue: [^nil].
	taskbarTask := TaskbarTask
		morph: self
		state: self taskbarState
		icon: (self iconNamed: self taskbarIconName)
		label: self taskbarLabel.
	^taskbarTask

and taskbarIconName does this:

taskbarIconName
	"Answer the icon for the receiver in a task bar."

	self model ifNotNil: [
		self model taskbarIconName
			ifNotNil: [ :aName | ^aName ] ].

	^ super taskbarIconName

Which means that the model of a window should respond to taskbarIconName. In Spec applications, the model is a SpWindowPresenter. That class does not implement taskbarIconName. It inherits it from Object:

taskbarIconName
	"Answer the icon for the receiver in a task bar
	or nil for the default."

	^self class taskbarIconName

and Object class implements:

taskbarIconName
	"Answer the icon for an instance of the receiver in a task bar"

	^#smallWindow

Due to this implementation, all Spec windows in the task bar are shown with the #smallWindow icon. And there is no way to change it!

Here is an example that does not work as expected. StPlaygroundPresenter has this method to specify the task bar icon:

windowIcon
	
	^ self application iconNamed: #workspace

but that method is never invoked, and therefore playground windows show up like this in the task bar:

Screenshot 2024-05-23 at 12 24 04

This icon should be shown according to the intentions of the StPlaygroundPresenter class:

Screenshot 2024-05-23 at 12 31 12

The consequence of this wrong behavior is that the section Setting the icon of the chapter Managing windows cannot be included. So @Ducasse I propose to take it out.

I will also create an issue for the wrong behaviour. We have to fix that. Actually there is a lot of related code that is not, or should not, be used anymore. We have to clean that up and make sure that Spec application developers (like me 😁) can specify which icon to show in the task bar.

@koendehondt
Copy link
Collaborator Author

@Ducasse and @estebanlm As announced in a previous comment, here is the issue to describe the problem and to suggest a solution: pharo-spec/Spec#1546.

@Ducasse
Copy link
Member

Ducasse commented May 25, 2024

Let us take it out (Setting the icon of the chapter Managing windows)

@koendehondt
Copy link
Collaborator Author

Let us take it out (Setting the icon of the chapter Managing windows)

Consider it done.

@koendehondt
Copy link
Collaborator Author

@estebanlm @Ducasse

there is an event (whenWillCloseDo:) to achieve the same now). it reaceives an event that can denyClose if needed.

Sadly there is a bug that does not allow me to write a decent example in the chapter: pharo-spec/Spec#1547

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

Successfully merging a pull request may close this issue.

3 participants