ponty / PyVirtualDisplay

Python wrapper for Xvfb, Xephyr and Xvnc
BSD 2-Clause "Simplified" License
697 stars 78 forks source link

'Display' object has no attribute 'is_started' #50

Closed garciparedes closed 2 years ago

garciparedes commented 4 years ago

Hi! After the new release (1.0.0) I'm getting the following error:

AttributeError: 'Display' object has no attribute 'is_started'

After looking for a solution for this problem, I realized that Display object is exposing the is_alive() method from the inner AbstractDisplay.

https://github.com/ponty/PyVirtualDisplay/blob/e92f53a3334d05e9969d7ce47d2df9312618892f/pyvirtualdisplay/display.py#L88

Then, my question is if I can start using this method obtaining the same behavior. The use case I was solving with is_started is to start the display if it's not already started:

display = Display(...)
...
if not display.is_started:
    display.start()
ponty commented 4 years ago

I just added is_started in 1.1 again.

is_started = start() was called. Xvfb can run or not.

is_alive = Xvfb was started and it is still running

garciparedes commented 4 years ago

Okay, thank you so much for your help @ponty!

Then, is_started attribute will be accessible from Display as in previous versions or only from AbstractDisplay?

The-Compiler commented 4 years ago

It looks like disp.is_started (with disp being a Display) works fine: aaf09b73faed1d6e9c1e06500578f8a374b7748e - so I'm guessing this can be closed?

ponty commented 4 years ago

I added it to Display class again. But it is only used internally for displaying better errror messages in case of programming errors. It has no use for library users, the caller always know if it called start() or not. If this makes confusion then it is better to remove it from public API. Can you show me a code example how it is called and why?

garciparedes commented 4 years ago

Yes, I'll explain you how I use the Display object. I've use it together with Bot instances.

What I want is to instantiate many Bot instances during the execution (You can think that I'm performing many scraping tasks which need to be reinitialized with a new Bot instance each a time. Nevertheless, I don't want to reinitialize the Display object each a time so I need to know if it's already started or not.

You can think one solution for my problem can be some like this:

display = Display()
display.start()

for stuff in stuff_container:
   bot = Bot(display)
   bot.perform(stuff)

Nevertheless, that approach is not so appropriate to solve my problem. In a nutshell, I don't want to have exposed the display instance outside the Bot class (because is one of many arguments needed to build a Bot).

Then, in a simplified way, to solve that issue I used a builder pattern:

class BotBuilder:
    def __init__(display=None):
        if display is None:
            display = Display()
        self.display = display

    def another():
        if not self.display.is_started:
            self.display.start()
        return Bot(display=self.display)

So, the way I use it becomes:

builder = BotBuilder()
for stuff in stuff_container:
   bot = builder.another()
   bot.perform(stuff)

The advantage of this approach is the isolation of the Display usage logic (and as I said previously, also of other arguments needed to build a Bot instance). So, by default is so easy to instantiate a new Bot object from BotBuilder. Another key point is that I don't want to start the screen if any Bot object needs to be used.

Then, the easiest way I've found to solve my problem is to be supported by display.is_started. 🙂

ponty commented 4 years ago
class BotBuilder:
    def __init__(display=None):
        if display is None:
            display = Display()
        self.display = display
        self.display.is_started = False # <---

    def another():
        if not self.display.is_started:
            self.display.start()
            self.display.is_started = True # <---
        return Bot(display=self.display)

If you add these 2 lines then you are independent from the Display class internals and the code is more readable: the meaning of is_started is explicit, there is no need for checking the implementation or documentation of an external library. As I wrote the caller always knows if it called start() or not.

garciparedes commented 4 years ago

Hi @ponty!

I agree with you in the sense that the responsibility to knowing if the start() method was call was performed or not should be the user Display.

Returning to my initial question and thinking about how to manage situations in which any external cause halts the display, what about if I replace display.is_started by display.is_alive()? I think that allowing to call two times to display.start() to recreate a halted display (due to a call to display.stop() or any external reason) supported by the diaplay.is_alive() method would be an interesting feature.

I'll try to explain me with an example. Now, if I perform:

display = Display()
display.is_alive()

I'll receive an AttributeError: 'NoneType' object has no attribute 'poll' but I think the expected behavior should be to return a False value. At this point, it would cover my use case in a proper way.

In addition and supported on that, it would be interesting to be able to perform display.start() any number of times when it is not alive. I mean, I expect that this will fail:

display = Display()
display.start()
display.start()

Nevertheless, I expect that this will work:

display = Display()
display.start()
display.stop()  # or any external reason who causes a display halting
display.start()

I could contribute to this project to recreate the behavior I explained. Do you agree with that? :wink:

ponty commented 4 years ago

I fixed is_alive(). is_alive() is used for testing wether Xvfb still runs.

I don't want to make it more complicated with multiple starts. If you need more start() then you can create more Display objects. I recommend using context manager which means that start() and stop() are not used.

with Display() as disp1: # context manager
        pass
    # do something
with Display() as disp2:
        pass
    # do something

I also removed is_started from the API.

ponty commented 2 years ago

No is_started from version 2.0.